From bdb4ccc5a0d994380bb5a6412c7b73f297d43a16 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Thu, 23 Jul 2026 16:00:46 +0000 Subject: [PATCH 1/7] Add `ucode configure skills` MCP-connection mode + status section Introduces `ucode configure skills` for managing a single Unity Catalog Skills MCP connection (`databricks-skill-registry`) whose scope is a set of `catalog.schema` locations, rendered as a multi-schema `?schema=` URL (aligns with the backend route in universe #2256555). - `build_skills_mcp_url(workspace, locations)`: repeated `?schema=`; empty list -> bare `/ai-gateway/skills/` (utility tools only). - Single `kind:"skills"` entry with `skill_locations` as the source of truth; the URL is derived and never parsed back. Every run rebuilds exactly one skills entry, preserving non-skills entries. - `--location a.b,c.d --mcp [--remove|--replace]` adds/removes/replaces the location set; `--remove`/`--replace` are mutually exclusive and require `--mcp`; a soft warning fires above 10 schemas. The bare form (no `--mcp`) registers the connection for the given schemas; download-only affordances (`--path`, `--yes`, a 3-part `.skill` location) without `--mcp` raise a clear "not available yet" error. - `alwaysLoad` is set on the skills entry for the Claude client only. - `ucode status` gains a top-level Skills section (Connection / Mode / Locations / Configured); the skills entry is excluded from each agent's per-client MCP-servers line. Also extracts `_setup_mcp_clients` from `configure_mcp_command` (shared by the mcp and skills commands) and adds an `always_load` keyword to `build_mcp_http_entry` (default off) as behavior-preserving prep. Co-authored-by: Isaac --- src/ucode/cli.py | 133 ++++++++++++++++++++++- src/ucode/databricks.py | 12 +++ src/ucode/mcp.py | 175 +++++++++++++++++++++++++++---- tests/test_cli.py | 185 ++++++++++++++++++++++++++++++++ tests/test_databricks.py | 16 +++ tests/test_mcp.py | 220 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 716 insertions(+), 25 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index a0ef7c4..d44fa6c 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -3,7 +3,7 @@ from __future__ import annotations -from typing import Annotated +from typing import Annotated, NamedTuple import typer from rich.panel import Panel @@ -53,7 +53,10 @@ ) from ucode.mcp import ( MCP_CLIENTS, + SKILLS_MCP_KIND, configure_mcp_command, + configure_skills_command, + configure_skills_mcp_command, purge_cross_workspace_mcp_residue, revert_mcp_configs, ) @@ -142,6 +145,52 @@ def _parse_agents_option(agents: str) -> list[str]: return tools +class SkillLocation(NamedTuple): + catalog: str + schema: str + skill: str | None + + @property + def schema_ref(self) -> str: + return f"{self.catalog}.{self.schema}" + + +def _parse_skill_locations(location: str, *, allow_skill: bool) -> list[SkillLocation]: + """Parse a comma-separated `--location` into `SkillLocation`s. + + Each entry is a 2-part `.` or 3-part + `..` dotted ref with non-empty parts. When + ``allow_skill`` is False the 3-part form is rejected. Duplicates are + dropped, preserving order. + """ + locations: list[SkillLocation] = [] + seen: set[SkillLocation] = set() + for raw in location.split(","): + raw = raw.strip() + if not raw: + continue + parts = raw.split(".") + if len(parts) not in (2, 3) or not all(part.strip() for part in parts): + raise RuntimeError( + f"--location entries must be `.[.]`, got `{raw}`." + ) + if len(parts) == 3 and not allow_skill: + raise RuntimeError( + f"`.{parts[2]}` names a single skill, which is download-only and not valid " + f"with --mcp; drop it to scope the connection to `{parts[0]}.{parts[1]}`." + ) + parsed = SkillLocation(parts[0], parts[1], parts[2] if len(parts) == 3 else None) + if parsed not in seen: + seen.add(parsed) + locations.append(parsed) + if not locations: + raise RuntimeError( + "No skills provided for --location. Use `.[.]`, " + "comma-separated for multiple." + ) + return locations + + def _parse_workspaces_option(workspaces: str) -> list[tuple[str, str | None]]: """Parse `--workspaces` into [(url, profile_name | None), ...]. @@ -697,7 +746,9 @@ def status() -> int: tool_mcp_servers = [ str(server.get("name")) for server in mcp_servers - if tool in (server.get("clients") or []) and server.get("name") + if tool in (server.get("clients") or []) + and server.get("name") + and server.get("kind") != SKILLS_MCP_KIND ] print_kv("MCP list command", str(MCP_CLIENTS[tool]["list_command"])) print_kv( @@ -707,6 +758,25 @@ def status() -> int: print_kv("Config file", str(config_path) if config_path.exists() else "missing") console.print() + print_heading("Skills") + skills_entry = next((s for s in mcp_servers if s.get("kind") == SKILLS_MCP_KIND), None) + if not skills_entry: + print_kv("Skills", "not configured") + else: + locations = skills_entry.get("skill_locations") or [] + print_kv("Connection", str(skills_entry.get("name"))) + print_kv("Mode", "mcp" if locations else "download") + print_kv( + "Locations", + ", ".join(locations) if locations else "none — utility tools only", + ) + displays = [ + str(MCP_CLIENTS[client]["display"]) + for client in (skills_entry.get("clients") or []) + if client in MCP_CLIENTS + ] + print_kv("Configured", ", ".join(displays) if displays else "none") + print_heading("Tracing") tracing = state.get("tracing") or {} if tracing.get("enabled"): @@ -1268,6 +1338,65 @@ def configure_mcp( raise typer.Exit(130) from None +@configure_app.command("skills") +def configure_skills( + location: Annotated[ + str, + typer.Option( + "--location", + help="Comma-separated `.` skill scopes (a single " + "`..` is download-only). Required.", + ), + ], + mcp: Annotated[ + bool, + typer.Option("--mcp", help="Mutate the skills MCP connection instead of downloading."), + ] = False, + remove: Annotated[ + bool, typer.Option("--remove", help="(--mcp) Remove the listed schemas from the set.") + ] = False, + replace: Annotated[ + bool, + typer.Option("--replace", help="(--mcp) Replace the set with exactly the listed schemas."), + ] = False, + path: Annotated[ + str | None, typer.Option("--path", help="(download) Project root; default user scope (~/).") + ] = None, + yes: Annotated[ + bool, + typer.Option("--yes", "-y", help="(download) Overwrite on collision without prompting."), + ] = False, +) -> None: + """Configure Databricks Skills for your coding tools.""" + try: + if remove and replace: + raise RuntimeError("Use either --remove or --replace, not both.") + if (remove or replace) and not mcp: + raise RuntimeError("--remove/--replace require --mcp.") + if path is not None and mcp: + raise RuntimeError("--path is not valid with --mcp.") + if mcp: + mode = "remove" if remove else "replace" if replace else "add" + locs = _parse_skill_locations(location, allow_skill=False) + configure_skills_mcp_command([loc.schema_ref for loc in locs], mode=mode) + else: + # Download mode lands in a later PR; until then, reject every + # download-only affordance rather than silently ignoring it. + locs = _parse_skill_locations(location, allow_skill=True) + if path is not None or yes or any(loc.skill for loc in locs): + raise RuntimeError( + "Download mode is not available yet; use --mcp to configure the skills " + "connection for the given schemas." + ) + configure_skills_command([loc.schema_ref for loc in locs]) + except RuntimeError as exc: + print_err(str(exc)) + raise typer.Exit(1) from None + except KeyboardInterrupt: + print_err("Interrupted.") + raise typer.Exit(130) from None + + @configure_app.command("tracing") def configure_tracing( disable: Annotated[ diff --git a/src/ucode/databricks.py b/src/ucode/databricks.py index b38bca9..7fcc263 100644 --- a/src/ucode/databricks.py +++ b/src/ucode/databricks.py @@ -1329,6 +1329,18 @@ def build_mcp_service_url(workspace: str, full_name: str) -> str: return f"{workspace}/ai-gateway/mcp-services/{full_name}" +def build_skills_mcp_url(workspace: str, locations: list[str]) -> str: + """Skills route with repeated ``?schema=`` scopes (universe PR #2256555). + + Empty ``locations`` -> the bare route (utility tools only). The trailing + slash is required by the Envoy prefix even with no query params. + """ + base = f"{workspace}/ai-gateway/skills/" + if not locations: + return base + return base + "?" + urlencode([("schema", loc) for loc in locations]) + + # Maps the gateway routing dialect a coding tool speaks to the Model Provider # Service `provider_type`s it can be backed by. claude speaks Anthropic's API, # which both the `anthropic` and `amazon_bedrock` provider types serve (Bedrock diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 6257458..9016dab 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -29,6 +29,7 @@ from ucode.databricks import ( apply_pat_environment, build_mcp_service_url, + build_skills_mcp_url, ensure_databricks_auth, get_databricks_token, list_databricks_apps, @@ -79,6 +80,9 @@ "list_command": "copilot mcp list", }, } +SKILLS_MCP_KIND = "skills" +SKILLS_MCP_SERVER_NAME = "databricks-skill-registry" +SKILLS_MCP_SCHEMA_SOFT_CAP = 10 EXTERNAL_MCP_SELECTION_PREFIX = "external:" SQL_MCP_VALUE = "managed:sql" GENIE_SPACE_SELECTION_PREFIX = "genie-space:" @@ -96,14 +100,17 @@ ) -def build_mcp_http_entry(url: str) -> dict: - return { +def build_mcp_http_entry(url: str, *, always_load: bool = False) -> dict: + entry: dict[str, Any] = { "type": "http", "url": url, "headers": { "Authorization": f"Bearer ${{{MCP_AUTH_TOKEN_ENV_VAR}}}", }, } + if always_load: + entry["alwaysLoad"] = True + return entry def add_claude_mcp_server(name: str, entry: dict, scope: str = MCP_USER_SCOPE) -> None: @@ -1037,7 +1044,10 @@ def apply_mcp_server_changes( url = server.get("url") if not isinstance(url, str) or not url: continue - entry = build_mcp_http_entry(url) + # alwaysLoad is Claude-only and rides the per-entry apply; other clients + # get URL only. The skills registry always loads so its utility tools + # are available without an explicit MCP-server mention. + entry = build_mcp_http_entry(url, always_load=server.get("kind") == SKILLS_MCP_KIND) for client in clients: configure_client_mcp_server(client, name, url, entry) changed = True @@ -1177,25 +1187,84 @@ def _resolve_location_mcp_servers( return working_servers -def configure_mcp_command(location: str | None = None, services: set[str] | None = None) -> int: - if services is not None and location is None: - # `--services` works standalone with full names (`system.ai.github`): the - # `.` to configure is derived from them. Bare short names - # (`github`) can't be located without `--location`. - schemas = {".".join(s.split(".")[:2]) for s in services if s.count(".") >= 2} - bare = sorted(s for s in services if s.count(".") < 2) - if bare: - raise RuntimeError( - "--services short names need --location (or pass full names like " - f"`system.ai.`): {', '.join(bare)}" - ) - if len(schemas) != 1: - raise RuntimeError( - "--services without --location must all share one `.` " - f"(got: {', '.join(sorted(schemas)) or 'none'}); pass --location instead." - ) - location = next(iter(schemas)) - state = load_state() +def _merge_clients(prior: list[str] | None, new: list[str]) -> list[str]: + """Order-preserving union of a prior client list with newly-configured ones.""" + prior = list(prior or []) + return prior + [c for c in new if c not in prior] + + +def _build_skills_entry(workspace: str, locations: list[str], clients: list[str]) -> dict: + """Canonical single skills-registry entry. ``skill_locations`` is the source + of truth; the URL is always derived from it, never parsed back.""" + return { + "name": SKILLS_MCP_SERVER_NAME, + "kind": SKILLS_MCP_KIND, + "skill_locations": list(locations), + "url": build_skills_mcp_url(workspace, locations), + "auth": f"env:{MCP_AUTH_TOKEN_ENV_VAR}", + "clients": clients, + } + + +def _resolve_skills_mcp_servers( + workspace: str, + clients: list[str], + locations: list[str], + original_servers: list[dict], +) -> list[dict]: + """Rebuild the MCP server list around exactly one skills entry. + + Drops every prior ``kind=="skills"`` entry and any entry named + ``SKILLS_MCP_SERVER_NAME`` (single-connection invariant; also sweeps up a + stray old-named entry via ``apply_mcp_server_changes``), keeps everything + else, and appends one rebuilt entry whose clients merge the prior skills + entry's clients with ``clients``. + """ + prior = next((s for s in original_servers if s.get("kind") == SKILLS_MCP_KIND), None) + merged = _merge_clients((prior or {}).get("clients"), clients) + kept = [ + s + for s in original_servers + if s.get("kind") != SKILLS_MCP_KIND and _server_name(s) != SKILLS_MCP_SERVER_NAME + ] + return [*kept, _build_skills_entry(workspace, locations, merged)] + + +def _dedupe_stable(values: list[str]) -> list[str]: + seen: set[str] = set() + result: list[str] = [] + for value in values: + if value not in seen: + seen.add(value) + result.append(value) + return result + + +def resolve_skill_location_set(current: list[str], requested: list[str], *, mode: str) -> list[str]: + """Apply a set mutation to the skill-location list. ``mode`` is one of + ``add`` (union), ``remove`` (difference), ``replace`` (deduped requested). + Order is stable throughout; backend assigns tool indices by URL position.""" + if mode == "remove": + return [c for c in current if c not in requested] + if mode == "replace": + return _dedupe_stable(requested) + return current + [r for r in requested if r not in current] + + +def _current_skill_locations(state: dict) -> list[str]: + """Read ``skill_locations`` off the single ``kind:"skills"`` entry, else ``[]``.""" + for server in state.get("mcp_servers") or []: + if server.get("kind") == SKILLS_MCP_KIND: + return list(server.get("skill_locations") or []) + return [] + + +def _setup_mcp_clients(state: dict, section: str) -> tuple[str, str | None, list[str]]: + """Validate the workspace, resolve configured MCP clients, and prepare auth. + + Returns ``(workspace, profile, clients)`` and prints the section header, the + "Configuring for" note, and a warning per configured-but-uninstalled client. + """ workspace = state.get("workspace") if not workspace: raise RuntimeError("Workspace is not configured. Run `ucode configure` first.") @@ -1223,7 +1292,7 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None apply_pat_environment(state) ensure_databricks_auth(workspace, profile) - print_section("MCP Servers") + print_section(section) client_names = ", ".join(str(MCP_CLIENTS[client]["display"]) for client in clients) print_note(f"Configuring for: {client_names}") for client in missing_clients: @@ -1231,6 +1300,29 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None f"{MCP_CLIENTS[client]['display']} is configured in ucode but not installed; " "skipping MCP config." ) + return workspace, profile, clients + + +def configure_mcp_command(location: str | None = None, services: set[str] | None = None) -> int: + if services is not None and location is None: + # `--services` works standalone with full names (`system.ai.github`): the + # `.` to configure is derived from them. Bare short names + # (`github`) can't be located without `--location`. + schemas = {".".join(s.split(".")[:2]) for s in services if s.count(".") >= 2} + bare = sorted(s for s in services if s.count(".") < 2) + if bare: + raise RuntimeError( + "--services short names need --location (or pass full names like " + f"`system.ai.`): {', '.join(bare)}" + ) + if len(schemas) != 1: + raise RuntimeError( + "--services without --location must all share one `.` " + f"(got: {', '.join(sorted(schemas)) or 'none'}); pass --location instead." + ) + location = next(iter(schemas)) + state = load_state() + workspace, profile, clients = _setup_mcp_clients(state, "MCP Servers") original_mcp_servers_for_location: list[dict] = list(state.get("mcp_servers") or []) if location is not None: @@ -1335,3 +1427,40 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None # User submitted the picker without toggling anything --> make it clear nothing was selected print_note("No MCP servers selected. Press space to toggle an item, then enter to save.") return 0 + + +def _apply_skills_connection( + state: dict, workspace: str, clients: list[str], locations: list[str] +) -> None: + """Rebuild the single skills connection for ``locations`` and persist it.""" + original = list(state.get("mcp_servers") or []) + working = _resolve_skills_mcp_servers(workspace, clients, locations, original) + changed = apply_mcp_server_changes(original, working, clients) + if changed or original != working: + state["mcp_servers"] = working + save_state(state) + print_success("Saved") + + +def configure_skills_mcp_command(locations: list[str], *, mode: str) -> int: + """Add/remove/replace the skills MCP connection's ``skill_locations`` set.""" + state = load_state() + workspace, _profile, clients = _setup_mcp_clients(state, "Skills MCP") + new_locations = resolve_skill_location_set( + _current_skill_locations(state), locations, mode=mode + ) + if len(new_locations) > SKILLS_MCP_SCHEMA_SOFT_CAP: + print_warning( + f"{len(new_locations)} schemas exceeds the recommended limit of " + f"{SKILLS_MCP_SCHEMA_SOFT_CAP}; registering anyway." + ) + _apply_skills_connection(state, workspace, clients, new_locations) + return 0 + + +def configure_skills_command(locations: list[str]) -> int: + """Bare-default entry point: register the skills connection for ``locations``.""" + state = load_state() + workspace, _profile, clients = _setup_mcp_clients(state, "Skills") + _apply_skills_connection(state, workspace, clients, locations) + return 0 diff --git a/tests/test_cli.py b/tests/test_cli.py index 76a2ec5..8599e23 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -342,6 +342,191 @@ def test_status_treats_available_tools_as_configured_agents(self): assert "https://example.databricks.com/ai-gateway/gemini" not in result.output +class TestConfigureSkillsCommand: + def test_default_calls_configure_skills_command(self): + with ( + patch("ucode.cli.configure_skills_command") as mock_default, + patch("ucode.cli.configure_skills_mcp_command") as mock_mcp, + ): + result = runner.invoke(app, ["configure", "skills", "--location", "a.b"]) + assert result.exit_code == 0, result.output + mock_default.assert_called_once_with(["a.b"]) + mock_mcp.assert_not_called() + + def test_mcp_flag_dispatches_add_mode(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke(app, ["configure", "skills", "--location", "a.b", "--mcp"]) + assert result.exit_code == 0, result.output + mock_mcp.assert_called_once_with(["a.b"], mode="add") + + def test_mcp_remove_dispatches_remove_mode(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke( + app, ["configure", "skills", "--location", "a.b", "--mcp", "--remove"] + ) + assert result.exit_code == 0, result.output + mock_mcp.assert_called_once_with(["a.b"], mode="remove") + + def test_mcp_replace_dispatches_replace_mode(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke( + app, ["configure", "skills", "--location", "a.b", "--mcp", "--replace"] + ) + assert result.exit_code == 0, result.output + mock_mcp.assert_called_once_with(["a.b"], mode="replace") + + def test_comma_location_yields_multiple_schemas(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke(app, ["configure", "skills", "--location", "a.b, c.d", "--mcp"]) + assert result.exit_code == 0, result.output + mock_mcp.assert_called_once_with(["a.b", "c.d"], mode="add") + + def test_remove_and_replace_together_exit_1(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke( + app, + ["configure", "skills", "--location", "a.b", "--mcp", "--remove", "--replace"], + ) + assert result.exit_code == 1 + mock_mcp.assert_not_called() + + def test_remove_without_mcp_exit_1(self): + with patch("ucode.cli.configure_skills_command") as mock_default: + result = runner.invoke(app, ["configure", "skills", "--location", "a.b", "--remove"]) + assert result.exit_code == 1 + mock_default.assert_not_called() + + def test_path_with_mcp_exit_1(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke( + app, ["configure", "skills", "--location", "a.b", "--mcp", "--path", "/tmp/p"] + ) + assert result.exit_code == 1 + mock_mcp.assert_not_called() + + def test_three_part_with_mcp_exit_1(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke(app, ["configure", "skills", "--location", "a.b.c", "--mcp"]) + assert result.exit_code == 1 + mock_mcp.assert_not_called() + + def test_three_part_without_mcp_is_download_only_exit_1(self): + with patch("ucode.cli.configure_skills_command") as mock_default: + result = runner.invoke(app, ["configure", "skills", "--location", "a.b.c"]) + assert result.exit_code == 1 + mock_default.assert_not_called() + + def test_path_without_mcp_is_download_only_exit_1(self): + with patch("ucode.cli.configure_skills_command") as mock_default: + result = runner.invoke( + app, ["configure", "skills", "--location", "a.b", "--path", "/tmp/p"] + ) + assert result.exit_code == 1 + mock_default.assert_not_called() + + def test_yes_without_mcp_is_download_only_exit_1(self): + with patch("ucode.cli.configure_skills_command") as mock_default: + result = runner.invoke(app, ["configure", "skills", "--location", "a.b", "--yes"]) + assert result.exit_code == 1 + mock_default.assert_not_called() + + def test_malformed_location_exit_1_names_location(self): + with patch("ucode.cli.configure_skills_command") as mock_default: + result = runner.invoke(app, ["configure", "skills", "--location", "justone"]) + assert result.exit_code == 1 + assert "--location" in _strip_ansi(result.output) + mock_default.assert_not_called() + + def test_missing_location_is_typer_usage_error(self): + result = runner.invoke(app, ["configure", "skills"]) + assert result.exit_code == 2 + + +class TestStatusSkillsSection: + def _run(self, state): + with patch("ucode.cli.load_state", return_value=state): + return runner.invoke(app, ["status"]) + + def test_not_configured_when_no_skills_entry(self): + result = self._run(MINIMAL_STATE) + assert result.exit_code == 0, result.output + out = _strip_ansi(result.output) + assert "Skills" in out + assert "not configured" in out + + def test_mcp_mode_rendering(self): + state = { + **MINIMAL_STATE, + "mcp_servers": [ + { + "name": "databricks-skill-registry", + "kind": "skills", + "skill_locations": ["main.default", "ml.prod"], + "url": "https://example.databricks.com/ai-gateway/skills/?schema=main.default&schema=ml.prod", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude", "codex"], + } + ], + } + result = self._run(state) + assert result.exit_code == 0, result.output + out = _strip_ansi(result.output) + assert "Connection: databricks-skill-registry" in out + assert "Mode: mcp" in out + assert "Locations: main.default, ml.prod" in out + assert "Configured: Claude Code, Codex" in out + + def test_download_mode_rendering(self): + state = { + **MINIMAL_STATE, + "mcp_servers": [ + { + "name": "databricks-skill-registry", + "kind": "skills", + "skill_locations": [], + "url": "https://example.databricks.com/ai-gateway/skills/", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + } + ], + } + result = self._run(state) + assert result.exit_code == 0, result.output + out = _strip_ansi(result.output) + assert "Mode: download" in out + assert "none — utility tools only" in out + + def test_skills_entry_absent_from_per_client_mcp_lines(self): + state = { + **MINIMAL_STATE, + "mcp_servers": [ + { + "name": "github-mcp", + "url": "https://example.databricks.com/api/2.0/mcp/external/github-mcp", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + }, + { + "name": "databricks-skill-registry", + "kind": "skills", + "skill_locations": ["main.default"], + "url": "https://example.databricks.com/ai-gateway/skills/?schema=main.default", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + }, + ], + } + result = self._run(state) + assert result.exit_code == 0, result.output + out = _strip_ansi(result.output) + # The skills registry appears only in the Skills section, never on a + # per-client "MCP servers:" line. + for line in out.splitlines(): + if "MCP servers:" in line: + assert "databricks-skill-registry" not in line + assert "Connection: databricks-skill-registry" in out + + class TestRevert: def test_reverts_mcp_configs_before_clearing_state(self): state = { diff --git a/tests/test_databricks.py b/tests/test_databricks.py index 40af048..0d4ebf5 100644 --- a/tests/test_databricks.py +++ b/tests/test_databricks.py @@ -21,6 +21,7 @@ build_databricks_cli_env, build_opencode_base_urls, build_shared_base_urls, + build_skills_mcp_url, build_tool_base_url, ensure_databricks_cli_version, ensure_pat_bearer, @@ -118,6 +119,21 @@ def test_codex_url_format(self): assert urls["codex"] == f"{WS}/ai-gateway/codex/v1" +class TestBuildSkillsMcpUrl: + def test_empty_locations_returns_bare_route(self): + assert build_skills_mcp_url(WS, []) == f"{WS}/ai-gateway/skills/" + + def test_single_location_appends_schema_query(self): + assert build_skills_mcp_url(WS, ["main.default"]) == ( + f"{WS}/ai-gateway/skills/?schema=main.default" + ) + + def test_multiple_locations_preserve_order(self): + assert build_skills_mcp_url(WS, ["a.b", "c.d"]) == ( + f"{WS}/ai-gateway/skills/?schema=a.b&schema=c.d" + ) + + class TestDiscoverClaudeModels: def test_selects_opus_4_8_when_advertised(self, monkeypatch): payload = { diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 81333c8..5a69624 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -25,6 +25,14 @@ def test_uses_oauth_token_header_reference(self): assert "oauth" not in entry assert "headersHelper" not in entry + def test_omits_always_load_by_default(self): + entry = mcp.build_mcp_http_entry(f"{WS}/ai-gateway/skills/") + assert "alwaysLoad" not in entry + + def test_sets_always_load_when_requested(self): + entry = mcp.build_mcp_http_entry(f"{WS}/ai-gateway/skills/", always_load=True) + assert entry["alwaysLoad"] is True + class TestAddClaudeMcpServer: def test_adds_user_scoped_json(self, monkeypatch): @@ -1642,6 +1650,218 @@ def test_full_names_spanning_multiple_schemas_without_location_raises(self): ) +def _find_skills(servers): + return [s for s in servers if s.get("kind") == mcp.SKILLS_MCP_KIND] + + +class TestResolveSkillLocationSet: + def test_add_is_order_preserving_union(self): + assert mcp.resolve_skill_location_set(["a.b"], ["c.d", "a.b"], mode="add") == [ + "a.b", + "c.d", + ] + + def test_add_to_empty(self): + assert mcp.resolve_skill_location_set([], ["a.b"], mode="add") == ["a.b"] + + def test_remove_is_difference(self): + assert mcp.resolve_skill_location_set(["a.b", "c.d"], ["c.d"], mode="remove") == ["a.b"] + + def test_replace_dedupes_preserving_order(self): + assert mcp.resolve_skill_location_set(["a.b"], ["x.y", "x.y", "z.w"], mode="replace") == [ + "x.y", + "z.w", + ] + + def test_replace_with_empty_clears(self): + assert mcp.resolve_skill_location_set(["a.b"], [], mode="replace") == [] + + +class TestResolveSkillsMcpServers: + def test_builds_single_canonical_entry(self): + servers = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["main.default"], []) + assert _find_skills(servers) == servers + entry = servers[0] + assert entry["name"] == mcp.SKILLS_MCP_SERVER_NAME + assert entry["kind"] == mcp.SKILLS_MCP_KIND + assert entry["skill_locations"] == ["main.default"] + assert entry["url"] == f"{WS}/ai-gateway/skills/?schema=main.default" + assert entry["auth"] == "env:OAUTH_TOKEN" + assert entry["clients"] == ["claude"] + + def test_keeps_other_entries_and_rebuilds_to_one_skills_entry(self): + service_entry = { + "name": "system-ai-github", + "url": f"{WS}/ai-gateway/mcp-services/system.ai.github", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + } + stale_skills = { + "name": mcp.SKILLS_MCP_SERVER_NAME, + "kind": mcp.SKILLS_MCP_KIND, + "skill_locations": ["old.one"], + "url": f"{WS}/ai-gateway/skills/?schema=old.one", + "auth": "env:OAUTH_TOKEN", + "clients": ["codex"], + } + servers = mcp._resolve_skills_mcp_servers( + WS, ["claude"], ["a.b"], [service_entry, stale_skills] + ) + assert service_entry in servers + skills = _find_skills(servers) + assert len(skills) == 1 + # Prior skills clients merge with the newly-configured ones (order-stable). + assert skills[0]["clients"] == ["codex", "claude"] + + def test_drops_entry_matching_skills_server_name_even_without_kind(self): + old_named = { + "name": mcp.SKILLS_MCP_SERVER_NAME, + "url": f"{WS}/ai-gateway/skills/", + "clients": ["claude"], + } + servers = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["a.b"], [old_named]) + assert len(servers) == 1 + assert servers[0]["kind"] == mcp.SKILLS_MCP_KIND + + def test_url_derives_from_locations_not_stale_url(self): + stale = { + "name": mcp.SKILLS_MCP_SERVER_NAME, + "kind": mcp.SKILLS_MCP_KIND, + "skill_locations": ["old.one"], + "url": f"{WS}/ai-gateway/skills/?schema=stale.value", + "clients": ["claude"], + } + servers = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["new.two"], [stale]) + assert servers[0]["url"] == f"{WS}/ai-gateway/skills/?schema=new.two" + + def test_empty_locations_yields_bare_route(self): + servers = mcp._resolve_skills_mcp_servers(WS, ["claude"], [], []) + assert servers[0]["skill_locations"] == [] + assert servers[0]["url"] == f"{WS}/ai-gateway/skills/" + + +def _skills_state(mcp_servers=None): + state = {"workspace": WS, "available_tools": ["claude"]} + if mcp_servers is not None: + state["mcp_servers"] = mcp_servers + return state + + +class TestConfigureSkillsMcpCommand: + def test_add_to_empty_registers_connection(self, monkeypatch): + saved_states: list[dict] = [] + configured: list[tuple[str, str, str, dict]] = [] + _stub_location_base(monkeypatch, _skills_state()) + monkeypatch.setattr( + mcp, + "configure_client_mcp_server", + lambda client, name, url, entry: configured.append((client, name, url, entry)) or [], + ) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + assert mcp.configure_skills_mcp_command(["a.b"], mode="add") == 0 + + skills = _find_skills(saved_states[-1]["mcp_servers"]) + assert len(skills) == 1 + assert skills[0]["skill_locations"] == ["a.b"] + assert skills[0]["url"] == f"{WS}/ai-gateway/skills/?schema=a.b" + # alwaysLoad rides the per-entry apply (Claude-only). + assert configured[0][3]["alwaysLoad"] is True + + def test_add_second_appends_in_order(self, monkeypatch): + saved_states: list[dict] = [] + prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a"], []) + _stub_location_base(monkeypatch, _skills_state(prior)) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + assert mcp.configure_skills_mcp_command(["B.b"], mode="add") == 0 + + assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["A.a", "B.b"] + + def test_remove_from_set(self, monkeypatch): + saved_states: list[dict] = [] + prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a", "B.b"], []) + _stub_location_base(monkeypatch, _skills_state(prior)) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + assert mcp.configure_skills_mcp_command(["B.b"], mode="remove") == 0 + + assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["A.a"] + + def test_replace_set(self, monkeypatch): + saved_states: list[dict] = [] + prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a", "B.b"], []) + _stub_location_base(monkeypatch, _skills_state(prior)) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + assert mcp.configure_skills_mcp_command(["X.x"], mode="replace") == 0 + + assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["X.x"] + + def test_remove_last_keeps_entry_with_bare_url(self, monkeypatch): + saved_states: list[dict] = [] + prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a"], []) + _stub_location_base(monkeypatch, _skills_state(prior)) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + assert mcp.configure_skills_mcp_command(["A.a"], mode="remove") == 0 + + skills = _find_skills(saved_states[-1]["mcp_servers"]) + assert len(skills) == 1 + assert skills[0]["skill_locations"] == [] + assert skills[0]["url"] == f"{WS}/ai-gateway/skills/" + + def test_preserves_mcp_service_entries_across_mutations(self, monkeypatch): + saved_states: list[dict] = [] + service_entry = { + "name": "system-ai-github", + "url": f"{WS}/ai-gateway/mcp-services/system.ai.github", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + } + prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a"], [service_entry]) + _stub_location_base(monkeypatch, _skills_state(prior)) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + assert mcp.configure_skills_mcp_command(["B.b"], mode="add") == 0 + + names = [s["name"] for s in saved_states[-1]["mcp_servers"]] + assert "system-ai-github" in names + assert names.count(mcp.SKILLS_MCP_SERVER_NAME) == 1 + + def test_soft_warns_over_schema_cap(self, monkeypatch, capsys): + saved_states: list[dict] = [] + _stub_location_base(monkeypatch, _skills_state()) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + many = [f"c.s{i}" for i in range(mcp.SKILLS_MCP_SCHEMA_SOFT_CAP + 1)] + assert mcp.configure_skills_mcp_command(many, mode="replace") == 0 + + assert len(_find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"]) == len(many) + out = capsys.readouterr().out + assert "recommended limit" in out + + +class TestConfigureSkillsCommand: + def test_bare_default_registers_connection(self, monkeypatch): + saved_states: list[dict] = [] + _stub_location_base(monkeypatch, _skills_state()) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + assert mcp.configure_skills_command(["a.b"]) == 0 + + skills = _find_skills(saved_states[-1]["mcp_servers"]) + assert len(skills) == 1 + assert skills[0]["skill_locations"] == ["a.b"] + + class TestRevertMcpConfigs: def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkeypatch): removed: list[tuple[str, str]] = [] From bf8f44970a146e386d87167e65e0b0386f1833d1 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Thu, 23 Jul 2026 16:55:20 +0000 Subject: [PATCH 2/7] Preserve the skills connection across `configure mcp` `configure mcp --location` (and the interactive picker) rebuilt the MCP server list from only the mcp-services they manage, so `apply_mcp_server_changes` treated the `kind:"skills"` entry as a removal and unregistered it from every agent. Both paths now carry an existing skills connection through untouched (the picker also hides it, since it is managed by `configure skills`). Co-authored-by: Isaac --- src/ucode/mcp.py | 25 +++++++++---- tests/test_mcp.py | 89 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 7 deletions(-) diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 9016dab..1728a26 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -1113,6 +1113,12 @@ def purge_cross_workspace_mcp_residue(state: dict, workspace: str) -> None: ) +def _skills_entries(servers: list[dict]) -> list[dict]: + """Skills connections carry their own lifecycle (``configure skills``); the + mcp commands preserve them untouched instead of treating them as removals.""" + return [s for s in servers if s.get("kind") == SKILLS_MCP_KIND] + + def _resolve_location_mcp_servers( workspace: str, profile: str | None, @@ -1123,9 +1129,10 @@ def _resolve_location_mcp_servers( ) -> list[dict]: """Build the desired MCP server list for ``--location .``. - Strict replacement: the returned list is exactly the mcp-services - discovered at ``location``. Any previously-registered MCP entries outside - that location are removed by ``apply_mcp_server_changes``. Raises ``RuntimeError`` for an invalid + Strict replacement for mcp-services: the returned list is exactly the ones + discovered at ``location`` (any previously-registered mcp-service outside it + is removed by ``apply_mcp_server_changes``), plus any existing skills + connection, preserved untouched. Raises ``RuntimeError`` for an invalid location (HTTP 404 from the listing API) or any other listing failure. When ``services`` is given, the discovered set is narrowed to exactly that @@ -1184,7 +1191,7 @@ def _resolve_location_mcp_servers( working_servers.append(original.copy()) else: working_servers.append(candidate) - return working_servers + return [*working_servers, *_skills_entries(original_servers)] def _merge_clients(prior: list[str] | None, new: list[str]) -> list[str]: @@ -1368,12 +1375,16 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None ) original_mcp_servers: list[dict] = list(state.get("mcp_servers") or []) - original_by_name = _servers_by_name(original_mcp_servers) + # Skills connections are managed by `configure skills`, so keep them out of + # the picker and carry them through untouched. + skills_servers = _skills_entries(original_mcp_servers) + picker_servers = [s for s in original_mcp_servers if s.get("kind") != SKILLS_MCP_KIND] + original_by_name = _servers_by_name(picker_servers) selections = prompt_for_mcp_server_choices( available_external_mcp_names, available_genie_mcp_servers, available_app_mcp_servers, - original_mcp_servers, + picker_servers, available_mcp_service_names, available_vector_search_servers, available_uc_functions_servers, @@ -1381,7 +1392,7 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None if selections is None: return 0 - working_mcp_servers: list[dict] = [] + working_mcp_servers: list[dict] = list(skills_servers) working_names: set[str] = set() add_selections: list[str] = [] for selection in selections: diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 5a69624..59191d4 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -825,6 +825,60 @@ def test_hints_when_no_selections_and_no_existing_servers(self, monkeypatch, cap assert "space to toggle" in output assert saved_states == [] + def test_preserves_skills_connection_and_hides_it_from_picker(self, monkeypatch): + """The picker manages mcp-services only; a skills connection is kept out + of the choices and never removed. Selecting an mcp-service saves both.""" + saved_states: list[dict] = [] + picker_servers: list[list[dict]] = [] + removed: list[tuple[str, str]] = [] + skills_entry = { + "name": mcp.SKILLS_MCP_SERVER_NAME, + "kind": mcp.SKILLS_MCP_KIND, + "skill_locations": ["main.default"], + "url": f"{WS}/ai-gateway/skills/?schema=main.default", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + } + + monkeypatch.setattr( + mcp, "load_state", lambda: {**CLAUDE_STATE, "mcp_servers": [skills_entry]} + ) + monkeypatch.setattr(mcp.shutil, "which", lambda binary: f"/usr/bin/{binary}") + monkeypatch.setattr(mcp, "ensure_databricks_auth", lambda workspace, profile=None: None) + monkeypatch.setattr(mcp, "available_mcp_clients", lambda: ["claude"]) + monkeypatch.setattr( + mcp, "discover_external_mcp_connection_names", lambda workspace, profile=None: [] + ) + monkeypatch.setattr(mcp, "discover_genie_mcp_servers", lambda workspace, profile=None: []) + monkeypatch.setattr(mcp, "discover_app_mcp_servers", lambda workspace, profile=None: []) + monkeypatch.setattr(mcp, "discover_mcp_service_names", lambda workspace, profile=None: []) + monkeypatch.setattr( + mcp, "discover_vector_search_mcp_servers", lambda workspace, profile=None: [] + ) + monkeypatch.setattr( + mcp, "discover_uc_functions_mcp_servers", lambda workspace, profile=None: [] + ) + monkeypatch.setattr( + mcp, + "prompt_for_mcp_server_choices", + lambda ext, genie, app, servers, *a, **kw: ( + picker_servers.append(servers) or [f"{mcp.MCP_ADD_PREFIX}{mcp.SQL_MCP_VALUE}"] + ), + ) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a: []) + 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() == 0 + + assert picker_servers == [[]] + assert removed == [] + assert skills_entry in saved_states[-1]["mcp_servers"] + def test_drops_stale_foreign_workspace_mcp_entries(self, monkeypatch, capsys): saved_states: list[dict] = [] cleanup_calls: list[tuple[str, str]] = [] @@ -1392,6 +1446,41 @@ def test_replaces_servers_outside_location(self, monkeypatch): }, ] + def test_preserves_skills_connection(self, monkeypatch): + """A skills connection is owned by `configure skills`, so `configure mcp + --location` must leave it registered rather than treating it as a removal.""" + saved_states: list[dict] = [] + removed: list[tuple[str, str]] = [] + skills_entry = { + "name": mcp.SKILLS_MCP_SERVER_NAME, + "kind": mcp.SKILLS_MCP_KIND, + "skill_locations": ["main.default"], + "url": f"{WS}/ai-gateway/skills/?schema=main.default", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + } + _stub_location_base( + monkeypatch, + {**CLAUDE_STATE, "mcp_servers": [skills_entry]}, + ) + monkeypatch.setattr( + mcp, + "list_mcp_services", + lambda workspace, token, parent: (["system.ai.github"], None), + ) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a: []) + 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="system.ai") == 0 + + assert removed == [] + assert _find_skills(saved_states[-1]["mcp_servers"]) == [skills_entry] + def test_existing_entry_gets_reconfigured_for_newly_added_clients(self, monkeypatch): """An entry registered before a second agent was configured should get registered for that agent on the next --location run.""" From 57c29b989078339d2c246f23a6e2b9825eeb4b81 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Thu, 23 Jul 2026 17:14:39 +0000 Subject: [PATCH 3/7] status: trim Skills section to locations + configured agents Drop the Connection line (fixed name, no signal) and the Mode line (inferred; deferred until download mode lands), and rename the locations line to `Skill MCP Locations` to distinguish it from download locations a later PR may track separately. Co-authored-by: Isaac --- src/ucode/cli.py | 4 +--- tests/test_cli.py | 17 +++++++---------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index d44fa6c..9394de1 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -764,10 +764,8 @@ def status() -> int: print_kv("Skills", "not configured") else: locations = skills_entry.get("skill_locations") or [] - print_kv("Connection", str(skills_entry.get("name"))) - print_kv("Mode", "mcp" if locations else "download") print_kv( - "Locations", + "Skill MCP Locations", ", ".join(locations) if locations else "none — utility tools only", ) displays = [ diff --git a/tests/test_cli.py b/tests/test_cli.py index 8599e23..151c050 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -454,7 +454,7 @@ def test_not_configured_when_no_skills_entry(self): assert "Skills" in out assert "not configured" in out - def test_mcp_mode_rendering(self): + def test_renders_locations_and_configured_agents(self): state = { **MINIMAL_STATE, "mcp_servers": [ @@ -471,12 +471,10 @@ def test_mcp_mode_rendering(self): result = self._run(state) assert result.exit_code == 0, result.output out = _strip_ansi(result.output) - assert "Connection: databricks-skill-registry" in out - assert "Mode: mcp" in out - assert "Locations: main.default, ml.prod" in out + assert "Skill MCP Locations: main.default, ml.prod" in out assert "Configured: Claude Code, Codex" in out - def test_download_mode_rendering(self): + def test_renders_placeholder_when_no_locations(self): state = { **MINIMAL_STATE, "mcp_servers": [ @@ -493,8 +491,7 @@ def test_download_mode_rendering(self): result = self._run(state) assert result.exit_code == 0, result.output out = _strip_ansi(result.output) - assert "Mode: download" in out - assert "none — utility tools only" in out + assert "Skill MCP Locations: none — utility tools only" in out def test_skills_entry_absent_from_per_client_mcp_lines(self): state = { @@ -519,12 +516,12 @@ def test_skills_entry_absent_from_per_client_mcp_lines(self): result = self._run(state) assert result.exit_code == 0, result.output out = _strip_ansi(result.output) - # The skills registry appears only in the Skills section, never on a - # per-client "MCP servers:" line. + # The skills registry is managed in the Skills section, never listed on + # a per-client "MCP servers:" line. for line in out.splitlines(): if "MCP servers:" in line: assert "databricks-skill-registry" not in line - assert "Connection: databricks-skill-registry" in out + assert "Skill MCP Locations: main.default" in out class TestRevert: From ba1f1b6e0c2ed5d20d19c9496355df19bdb074e5 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Thu, 23 Jul 2026 18:26:29 +0000 Subject: [PATCH 4/7] Address review: scope skills PR to MCP mode only Defer all download scaffolding to the later download-mode PR and apply review cleanups: - CLI: drop --path/--yes and 3-part `.skill` parsing; --location takes only `.` refs. Non---mcp invocations return a clear "not available yet" error until download mode lands. - Remove the bare `configure_skills_command` entry point and the `SkillLocation` NamedTuple. - Drop the client-side schema soft-cap; let the backend enforce limits. - Rename `_apply_skills_connection` -> `_update_skills_mcp`; group the skills helpers just above `configure_skills_mcp_command`. - `resolve_skill_location_set` dedupes via `dict.fromkeys`. - status: rename locals to `skill_mcp_entry` / `configured_agents`. - Tighten `build_skills_mcp_url` docstring and a couple of comments. Co-authored-by: Isaac --- src/ucode/cli.py | 103 +++++++++++----------------------------- src/ucode/databricks.py | 7 +-- src/ucode/mcp.py | 73 +++++++++------------------- tests/test_cli.py | 56 ++++------------------ tests/test_mcp.py | 27 ----------- 5 files changed, 64 insertions(+), 202 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 9394de1..6d591bd 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -3,7 +3,7 @@ from __future__ import annotations -from typing import Annotated, NamedTuple +from typing import Annotated import typer from rich.panel import Panel @@ -55,7 +55,6 @@ MCP_CLIENTS, SKILLS_MCP_KIND, configure_mcp_command, - configure_skills_command, configure_skills_mcp_command, purge_cross_workspace_mcp_residue, revert_mcp_configs, @@ -145,47 +144,22 @@ def _parse_agents_option(agents: str) -> list[str]: return tools -class SkillLocation(NamedTuple): - catalog: str - schema: str - skill: str | None - - @property - def schema_ref(self) -> str: - return f"{self.catalog}.{self.schema}" - - -def _parse_skill_locations(location: str, *, allow_skill: bool) -> list[SkillLocation]: - """Parse a comma-separated `--location` into `SkillLocation`s. - - Each entry is a 2-part `.` or 3-part - `..` dotted ref with non-empty parts. When - ``allow_skill`` is False the 3-part form is rejected. Duplicates are - dropped, preserving order. - """ - locations: list[SkillLocation] = [] - seen: set[SkillLocation] = set() +def _parse_skill_locations(location: str) -> list[str]: + """Parse a comma-separated `--location` into `.` refs, + dropping duplicates while preserving order.""" + locations: list[str] = [] for raw in location.split(","): raw = raw.strip() if not raw: continue parts = raw.split(".") - if len(parts) not in (2, 3) or not all(part.strip() for part in parts): - raise RuntimeError( - f"--location entries must be `.[.]`, got `{raw}`." - ) - if len(parts) == 3 and not allow_skill: - raise RuntimeError( - f"`.{parts[2]}` names a single skill, which is download-only and not valid " - f"with --mcp; drop it to scope the connection to `{parts[0]}.{parts[1]}`." - ) - parsed = SkillLocation(parts[0], parts[1], parts[2] if len(parts) == 3 else None) - if parsed not in seen: - seen.add(parsed) - locations.append(parsed) + if len(parts) != 2 or not all(part.strip() for part in parts): + raise RuntimeError(f"--location entries must be `.`, got `{raw}`.") + if raw not in locations: + locations.append(raw) if not locations: raise RuntimeError( - "No skills provided for --location. Use `.[.]`, " + "No schemas provided for --location. Use `.`, " "comma-separated for multiple." ) return locations @@ -759,21 +733,21 @@ def status() -> int: console.print() print_heading("Skills") - skills_entry = next((s for s in mcp_servers if s.get("kind") == SKILLS_MCP_KIND), None) - if not skills_entry: + skill_mcp_entry = next((s for s in mcp_servers if s.get("kind") == SKILLS_MCP_KIND), None) + if not skill_mcp_entry: print_kv("Skills", "not configured") else: - locations = skills_entry.get("skill_locations") or [] + locations = skill_mcp_entry.get("skill_locations") or [] print_kv( "Skill MCP Locations", ", ".join(locations) if locations else "none — utility tools only", ) - displays = [ + configured_agents = [ str(MCP_CLIENTS[client]["display"]) - for client in (skills_entry.get("clients") or []) + for client in (skill_mcp_entry.get("clients") or []) if client in MCP_CLIENTS ] - print_kv("Configured", ", ".join(displays) if displays else "none") + print_kv("Configured", ", ".join(configured_agents) if configured_agents else "none") print_heading("Tracing") tracing = state.get("tracing") or {} @@ -1340,53 +1314,32 @@ def configure_mcp( def configure_skills( location: Annotated[ str, - typer.Option( - "--location", - help="Comma-separated `.` skill scopes (a single " - "`..` is download-only). Required.", - ), + typer.Option("--location", help="Comma-separated `.` skill scopes."), ], mcp: Annotated[ bool, - typer.Option("--mcp", help="Mutate the skills MCP connection instead of downloading."), + typer.Option( + "--mcp", help="Manage the skills MCP connection (required until download mode lands)." + ), ] = False, remove: Annotated[ - bool, typer.Option("--remove", help="(--mcp) Remove the listed schemas from the set.") + bool, typer.Option("--remove", help="With --mcp: remove the listed schemas from the set.") ] = False, replace: Annotated[ bool, - typer.Option("--replace", help="(--mcp) Replace the set with exactly the listed schemas."), - ] = False, - path: Annotated[ - str | None, typer.Option("--path", help="(download) Project root; default user scope (~/).") - ] = None, - yes: Annotated[ - bool, - typer.Option("--yes", "-y", help="(download) Overwrite on collision without prompting."), + typer.Option( + "--replace", help="With --mcp: replace the set with exactly the listed schemas." + ), ] = False, ) -> None: """Configure Databricks Skills for your coding tools.""" try: + if not mcp: + raise RuntimeError("Download mode is not available yet; pass --mcp for now.") if remove and replace: raise RuntimeError("Use either --remove or --replace, not both.") - if (remove or replace) and not mcp: - raise RuntimeError("--remove/--replace require --mcp.") - if path is not None and mcp: - raise RuntimeError("--path is not valid with --mcp.") - if mcp: - mode = "remove" if remove else "replace" if replace else "add" - locs = _parse_skill_locations(location, allow_skill=False) - configure_skills_mcp_command([loc.schema_ref for loc in locs], mode=mode) - else: - # Download mode lands in a later PR; until then, reject every - # download-only affordance rather than silently ignoring it. - locs = _parse_skill_locations(location, allow_skill=True) - if path is not None or yes or any(loc.skill for loc in locs): - raise RuntimeError( - "Download mode is not available yet; use --mcp to configure the skills " - "connection for the given schemas." - ) - configure_skills_command([loc.schema_ref for loc in locs]) + mode = "remove" if remove else "replace" if replace else "add" + configure_skills_mcp_command(_parse_skill_locations(location), mode=mode) except RuntimeError as exc: print_err(str(exc)) raise typer.Exit(1) from None diff --git a/src/ucode/databricks.py b/src/ucode/databricks.py index 7fcc263..a337c02 100644 --- a/src/ucode/databricks.py +++ b/src/ucode/databricks.py @@ -1330,10 +1330,11 @@ def build_mcp_service_url(workspace: str, full_name: str) -> str: def build_skills_mcp_url(workspace: str, locations: list[str]) -> str: - """Skills route with repeated ``?schema=`` scopes (universe PR #2256555). + """Skills route with one ``?schema=`` scope per location. The trailing slash + is required by the Envoy prefix even with no query params. - Empty ``locations`` -> the bare route (utility tools only). The trailing - slash is required by the Envoy prefix even with no query params. + [] -> ``.../ai-gateway/skills/`` + ["main.default", "ml.a"] -> ``.../ai-gateway/skills/?schema=main.default&schema=ml.a`` """ base = f"{workspace}/ai-gateway/skills/" if not locations: diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 1728a26..a0af705 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -82,7 +82,6 @@ } SKILLS_MCP_KIND = "skills" SKILLS_MCP_SERVER_NAME = "databricks-skill-registry" -SKILLS_MCP_SCHEMA_SOFT_CAP = 10 EXTERNAL_MCP_SELECTION_PREFIX = "external:" SQL_MCP_VALUE = "managed:sql" GENIE_SPACE_SELECTION_PREFIX = "genie-space:" @@ -1044,9 +1043,8 @@ def apply_mcp_server_changes( url = server.get("url") if not isinstance(url, str) or not url: continue - # alwaysLoad is Claude-only and rides the per-entry apply; other clients - # get URL only. The skills registry always loads so its utility tools - # are available without an explicit MCP-server mention. + # alwaysLoad (Claude-only) keeps the skills registry's utility tools + # discoverable without an explicit mention; other clients ignore it. entry = build_mcp_http_entry(url, always_load=server.get("kind") == SKILLS_MCP_KIND) for client in clients: configure_client_mcp_server(client, name, url, entry) @@ -1114,8 +1112,6 @@ def purge_cross_workspace_mcp_residue(state: dict, workspace: str) -> None: def _skills_entries(servers: list[dict]) -> list[dict]: - """Skills connections carry their own lifecycle (``configure skills``); the - mcp commands preserve them untouched instead of treating them as removals.""" return [s for s in servers if s.get("kind") == SKILLS_MCP_KIND] @@ -1237,35 +1233,6 @@ def _resolve_skills_mcp_servers( return [*kept, _build_skills_entry(workspace, locations, merged)] -def _dedupe_stable(values: list[str]) -> list[str]: - seen: set[str] = set() - result: list[str] = [] - for value in values: - if value not in seen: - seen.add(value) - result.append(value) - return result - - -def resolve_skill_location_set(current: list[str], requested: list[str], *, mode: str) -> list[str]: - """Apply a set mutation to the skill-location list. ``mode`` is one of - ``add`` (union), ``remove`` (difference), ``replace`` (deduped requested). - Order is stable throughout; backend assigns tool indices by URL position.""" - if mode == "remove": - return [c for c in current if c not in requested] - if mode == "replace": - return _dedupe_stable(requested) - return current + [r for r in requested if r not in current] - - -def _current_skill_locations(state: dict) -> list[str]: - """Read ``skill_locations`` off the single ``kind:"skills"`` entry, else ``[]``.""" - for server in state.get("mcp_servers") or []: - if server.get("kind") == SKILLS_MCP_KIND: - return list(server.get("skill_locations") or []) - return [] - - def _setup_mcp_clients(state: dict, section: str) -> tuple[str, str | None, list[str]]: """Validate the workspace, resolve configured MCP clients, and prepare auth. @@ -1440,7 +1407,26 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None return 0 -def _apply_skills_connection( +def _current_skill_locations(state: dict) -> list[str]: + """Read ``skill_locations`` off the single ``kind:"skills"`` entry, else ``[]``.""" + for server in state.get("mcp_servers") or []: + if server.get("kind") == SKILLS_MCP_KIND: + return list(server.get("skill_locations") or []) + return [] + + +def resolve_skill_location_set(current: list[str], requested: list[str], *, mode: str) -> list[str]: + """Apply a set mutation to the skill-location list. ``mode`` is one of + ``add`` (union), ``remove`` (difference), ``replace`` (deduped requested). + Order is stable throughout; backend assigns tool indices by URL position.""" + if mode == "remove": + return [c for c in current if c not in requested] + if mode == "replace": + return list(dict.fromkeys(requested)) + return current + [r for r in requested if r not in current] + + +def _update_skills_mcp( state: dict, workspace: str, clients: list[str], locations: list[str] ) -> None: """Rebuild the single skills connection for ``locations`` and persist it.""" @@ -1460,18 +1446,5 @@ def configure_skills_mcp_command(locations: list[str], *, mode: str) -> int: new_locations = resolve_skill_location_set( _current_skill_locations(state), locations, mode=mode ) - if len(new_locations) > SKILLS_MCP_SCHEMA_SOFT_CAP: - print_warning( - f"{len(new_locations)} schemas exceeds the recommended limit of " - f"{SKILLS_MCP_SCHEMA_SOFT_CAP}; registering anyway." - ) - _apply_skills_connection(state, workspace, clients, new_locations) - return 0 - - -def configure_skills_command(locations: list[str]) -> int: - """Bare-default entry point: register the skills connection for ``locations``.""" - state = load_state() - workspace, _profile, clients = _setup_mcp_clients(state, "Skills") - _apply_skills_connection(state, workspace, clients, locations) + _update_skills_mcp(state, workspace, clients, new_locations) return 0 diff --git a/tests/test_cli.py b/tests/test_cli.py index 151c050..cca6941 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -343,16 +343,6 @@ def test_status_treats_available_tools_as_configured_agents(self): class TestConfigureSkillsCommand: - def test_default_calls_configure_skills_command(self): - with ( - patch("ucode.cli.configure_skills_command") as mock_default, - patch("ucode.cli.configure_skills_mcp_command") as mock_mcp, - ): - result = runner.invoke(app, ["configure", "skills", "--location", "a.b"]) - assert result.exit_code == 0, result.output - mock_default.assert_called_once_with(["a.b"]) - mock_mcp.assert_not_called() - def test_mcp_flag_dispatches_add_mode(self): with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: result = runner.invoke(app, ["configure", "skills", "--location", "a.b", "--mcp"]) @@ -381,61 +371,33 @@ def test_comma_location_yields_multiple_schemas(self): assert result.exit_code == 0, result.output mock_mcp.assert_called_once_with(["a.b", "c.d"], mode="add") - def test_remove_and_replace_together_exit_1(self): + def test_without_mcp_is_not_implemented_exit_1(self): with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: - result = runner.invoke( - app, - ["configure", "skills", "--location", "a.b", "--mcp", "--remove", "--replace"], - ) + result = runner.invoke(app, ["configure", "skills", "--location", "a.b"]) assert result.exit_code == 1 mock_mcp.assert_not_called() - def test_remove_without_mcp_exit_1(self): - with patch("ucode.cli.configure_skills_command") as mock_default: - result = runner.invoke(app, ["configure", "skills", "--location", "a.b", "--remove"]) - assert result.exit_code == 1 - mock_default.assert_not_called() - - def test_path_with_mcp_exit_1(self): + def test_remove_and_replace_together_exit_1(self): with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--mcp", "--path", "/tmp/p"] + app, + ["configure", "skills", "--location", "a.b", "--mcp", "--remove", "--replace"], ) assert result.exit_code == 1 mock_mcp.assert_not_called() - def test_three_part_with_mcp_exit_1(self): + def test_three_part_location_exit_1(self): with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: result = runner.invoke(app, ["configure", "skills", "--location", "a.b.c", "--mcp"]) assert result.exit_code == 1 mock_mcp.assert_not_called() - def test_three_part_without_mcp_is_download_only_exit_1(self): - with patch("ucode.cli.configure_skills_command") as mock_default: - result = runner.invoke(app, ["configure", "skills", "--location", "a.b.c"]) - assert result.exit_code == 1 - mock_default.assert_not_called() - - def test_path_without_mcp_is_download_only_exit_1(self): - with patch("ucode.cli.configure_skills_command") as mock_default: - result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--path", "/tmp/p"] - ) - assert result.exit_code == 1 - mock_default.assert_not_called() - - def test_yes_without_mcp_is_download_only_exit_1(self): - with patch("ucode.cli.configure_skills_command") as mock_default: - result = runner.invoke(app, ["configure", "skills", "--location", "a.b", "--yes"]) - assert result.exit_code == 1 - mock_default.assert_not_called() - def test_malformed_location_exit_1_names_location(self): - with patch("ucode.cli.configure_skills_command") as mock_default: - result = runner.invoke(app, ["configure", "skills", "--location", "justone"]) + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke(app, ["configure", "skills", "--location", "justone", "--mcp"]) assert result.exit_code == 1 assert "--location" in _strip_ansi(result.output) - mock_default.assert_not_called() + mock_mcp.assert_not_called() def test_missing_location_is_typer_usage_error(self): result = runner.invoke(app, ["configure", "skills"]) diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 59191d4..ca4a581 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -1923,33 +1923,6 @@ def test_preserves_mcp_service_entries_across_mutations(self, monkeypatch): assert "system-ai-github" in names assert names.count(mcp.SKILLS_MCP_SERVER_NAME) == 1 - def test_soft_warns_over_schema_cap(self, monkeypatch, capsys): - saved_states: list[dict] = [] - _stub_location_base(monkeypatch, _skills_state()) - monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) - monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - - many = [f"c.s{i}" for i in range(mcp.SKILLS_MCP_SCHEMA_SOFT_CAP + 1)] - assert mcp.configure_skills_mcp_command(many, mode="replace") == 0 - - assert len(_find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"]) == len(many) - out = capsys.readouterr().out - assert "recommended limit" in out - - -class TestConfigureSkillsCommand: - def test_bare_default_registers_connection(self, monkeypatch): - saved_states: list[dict] = [] - _stub_location_base(monkeypatch, _skills_state()) - monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) - monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - - assert mcp.configure_skills_command(["a.b"]) == 0 - - skills = _find_skills(saved_states[-1]["mcp_servers"]) - assert len(skills) == 1 - assert skills[0]["skill_locations"] == ["a.b"] - class TestRevertMcpConfigs: def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkeypatch): From f7628f4a0b520f613321b15e88692fc167111895 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Thu, 23 Jul 2026 18:51:48 +0000 Subject: [PATCH 5/7] Address review: drop redundant dedup, group skills helpers - `resolve_skill_location_set` no longer de-dupes the `replace` set; the CLI parser (`_parse_skill_locations`) already drops duplicates, so it just returns `requested`. - Move `_merge_clients`, `_build_skills_entry`, and `_resolve_skills_mcp_servers` down to sit with the rest of the skills helpers just above `configure_skills_mcp_command`. Co-authored-by: Isaac --- src/ucode/mcp.py | 95 ++++++++++++++++++++++++----------------------- tests/test_mcp.py | 4 +- 2 files changed, 50 insertions(+), 49 deletions(-) diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index a0af705..504c244 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -1190,49 +1190,6 @@ def _resolve_location_mcp_servers( return [*working_servers, *_skills_entries(original_servers)] -def _merge_clients(prior: list[str] | None, new: list[str]) -> list[str]: - """Order-preserving union of a prior client list with newly-configured ones.""" - prior = list(prior or []) - return prior + [c for c in new if c not in prior] - - -def _build_skills_entry(workspace: str, locations: list[str], clients: list[str]) -> dict: - """Canonical single skills-registry entry. ``skill_locations`` is the source - of truth; the URL is always derived from it, never parsed back.""" - return { - "name": SKILLS_MCP_SERVER_NAME, - "kind": SKILLS_MCP_KIND, - "skill_locations": list(locations), - "url": build_skills_mcp_url(workspace, locations), - "auth": f"env:{MCP_AUTH_TOKEN_ENV_VAR}", - "clients": clients, - } - - -def _resolve_skills_mcp_servers( - workspace: str, - clients: list[str], - locations: list[str], - original_servers: list[dict], -) -> list[dict]: - """Rebuild the MCP server list around exactly one skills entry. - - Drops every prior ``kind=="skills"`` entry and any entry named - ``SKILLS_MCP_SERVER_NAME`` (single-connection invariant; also sweeps up a - stray old-named entry via ``apply_mcp_server_changes``), keeps everything - else, and appends one rebuilt entry whose clients merge the prior skills - entry's clients with ``clients``. - """ - prior = next((s for s in original_servers if s.get("kind") == SKILLS_MCP_KIND), None) - merged = _merge_clients((prior or {}).get("clients"), clients) - kept = [ - s - for s in original_servers - if s.get("kind") != SKILLS_MCP_KIND and _server_name(s) != SKILLS_MCP_SERVER_NAME - ] - return [*kept, _build_skills_entry(workspace, locations, merged)] - - def _setup_mcp_clients(state: dict, section: str) -> tuple[str, str | None, list[str]]: """Validate the workspace, resolve configured MCP clients, and prepare auth. @@ -1407,6 +1364,49 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None return 0 +def _merge_clients(prior: list[str] | None, new: list[str]) -> list[str]: + """Order-preserving union of a prior client list with newly-configured ones.""" + prior = list(prior or []) + return prior + [c for c in new if c not in prior] + + +def _build_skills_entry(workspace: str, locations: list[str], clients: list[str]) -> dict: + """Canonical single skills-registry entry. ``skill_locations`` is the source + of truth; the URL is always derived from it, never parsed back.""" + return { + "name": SKILLS_MCP_SERVER_NAME, + "kind": SKILLS_MCP_KIND, + "skill_locations": list(locations), + "url": build_skills_mcp_url(workspace, locations), + "auth": f"env:{MCP_AUTH_TOKEN_ENV_VAR}", + "clients": clients, + } + + +def _resolve_skills_mcp_servers( + workspace: str, + clients: list[str], + locations: list[str], + original_servers: list[dict], +) -> list[dict]: + """Rebuild the MCP server list around exactly one skills entry. + + Drops every prior ``kind=="skills"`` entry and any entry named + ``SKILLS_MCP_SERVER_NAME`` (single-connection invariant; also sweeps up a + stray old-named entry via ``apply_mcp_server_changes``), keeps everything + else, and appends one rebuilt entry whose clients merge the prior skills + entry's clients with ``clients``. + """ + prior = next((s for s in original_servers if s.get("kind") == SKILLS_MCP_KIND), None) + merged = _merge_clients((prior or {}).get("clients"), clients) + kept = [ + s + for s in original_servers + if s.get("kind") != SKILLS_MCP_KIND and _server_name(s) != SKILLS_MCP_SERVER_NAME + ] + return [*kept, _build_skills_entry(workspace, locations, merged)] + + def _current_skill_locations(state: dict) -> list[str]: """Read ``skill_locations`` off the single ``kind:"skills"`` entry, else ``[]``.""" for server in state.get("mcp_servers") or []: @@ -1416,13 +1416,14 @@ def _current_skill_locations(state: dict) -> list[str]: def resolve_skill_location_set(current: list[str], requested: list[str], *, mode: str) -> list[str]: - """Apply a set mutation to the skill-location list. ``mode`` is one of - ``add`` (union), ``remove`` (difference), ``replace`` (deduped requested). - Order is stable throughout; backend assigns tool indices by URL position.""" + """Apply a set mutation to the skill-location list (``requested`` is already + de-duplicated by the CLI parser). ``mode`` is one of ``add`` (union), + ``remove`` (difference), ``replace`` (exactly ``requested``). Order is stable; + the backend assigns tool indices by URL position.""" if mode == "remove": return [c for c in current if c not in requested] if mode == "replace": - return list(dict.fromkeys(requested)) + return requested return current + [r for r in requested if r not in current] diff --git a/tests/test_mcp.py b/tests/test_mcp.py index ca4a581..6f06f82 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -1756,8 +1756,8 @@ def test_add_to_empty(self): def test_remove_is_difference(self): assert mcp.resolve_skill_location_set(["a.b", "c.d"], ["c.d"], mode="remove") == ["a.b"] - def test_replace_dedupes_preserving_order(self): - assert mcp.resolve_skill_location_set(["a.b"], ["x.y", "x.y", "z.w"], mode="replace") == [ + def test_replace_swaps_in_requested(self): + assert mcp.resolve_skill_location_set(["a.b"], ["x.y", "z.w"], mode="replace") == [ "x.y", "z.w", ] From 85c375d31a45fb774840a53b6ba7db5b26a32033 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Thu, 23 Jul 2026 21:54:11 +0000 Subject: [PATCH 6/7] skills: make --location override-only, drop --remove/--replace Per review, keep the MCP-mode surface minimal: --location now sets the skills connection's schema set to exactly the listed locations, replacing any prior set, instead of add/remove/replace mutations. Removes the --remove/--replace flags and mode dispatch, and the now-dead resolve_skill_location_set / _current_skill_locations helpers. Co-authored-by: Isaac --- src/ucode/cli.py | 20 +++++--------- src/ucode/mcp.py | 30 +++------------------ tests/test_cli.py | 31 +++------------------- tests/test_mcp.py | 66 +++++++---------------------------------------- 4 files changed, 22 insertions(+), 125 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 6d591bd..36afde4 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -1322,24 +1322,16 @@ def configure_skills( "--mcp", help="Manage the skills MCP connection (required until download mode lands)." ), ] = False, - remove: Annotated[ - bool, typer.Option("--remove", help="With --mcp: remove the listed schemas from the set.") - ] = False, - replace: Annotated[ - bool, - typer.Option( - "--replace", help="With --mcp: replace the set with exactly the listed schemas." - ), - ] = False, ) -> None: - """Configure Databricks Skills for your coding tools.""" + """Configure Databricks Skills for your coding tools. + + ``--location`` sets the skills MCP connection's scope to exactly the listed + schemas, replacing any previous set. + """ try: if not mcp: raise RuntimeError("Download mode is not available yet; pass --mcp for now.") - if remove and replace: - raise RuntimeError("Use either --remove or --replace, not both.") - mode = "remove" if remove else "replace" if replace else "add" - configure_skills_mcp_command(_parse_skill_locations(location), mode=mode) + configure_skills_mcp_command(_parse_skill_locations(location)) except RuntimeError as exc: print_err(str(exc)) raise typer.Exit(1) from None diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 504c244..9ab931f 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -1407,26 +1407,6 @@ def _resolve_skills_mcp_servers( return [*kept, _build_skills_entry(workspace, locations, merged)] -def _current_skill_locations(state: dict) -> list[str]: - """Read ``skill_locations`` off the single ``kind:"skills"`` entry, else ``[]``.""" - for server in state.get("mcp_servers") or []: - if server.get("kind") == SKILLS_MCP_KIND: - return list(server.get("skill_locations") or []) - return [] - - -def resolve_skill_location_set(current: list[str], requested: list[str], *, mode: str) -> list[str]: - """Apply a set mutation to the skill-location list (``requested`` is already - de-duplicated by the CLI parser). ``mode`` is one of ``add`` (union), - ``remove`` (difference), ``replace`` (exactly ``requested``). Order is stable; - the backend assigns tool indices by URL position.""" - if mode == "remove": - return [c for c in current if c not in requested] - if mode == "replace": - return requested - return current + [r for r in requested if r not in current] - - def _update_skills_mcp( state: dict, workspace: str, clients: list[str], locations: list[str] ) -> None: @@ -1440,12 +1420,10 @@ def _update_skills_mcp( print_success("Saved") -def configure_skills_mcp_command(locations: list[str], *, mode: str) -> int: - """Add/remove/replace the skills MCP connection's ``skill_locations`` set.""" +def configure_skills_mcp_command(locations: list[str]) -> int: + """Set the skills MCP connection's ``skill_locations`` to exactly ``locations``, + replacing any previous set.""" state = load_state() workspace, _profile, clients = _setup_mcp_clients(state, "Skills MCP") - new_locations = resolve_skill_location_set( - _current_skill_locations(state), locations, mode=mode - ) - _update_skills_mcp(state, workspace, clients, new_locations) + _update_skills_mcp(state, workspace, clients, locations) return 0 diff --git a/tests/test_cli.py b/tests/test_cli.py index cca6941..d734111 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -343,33 +343,17 @@ def test_status_treats_available_tools_as_configured_agents(self): class TestConfigureSkillsCommand: - def test_mcp_flag_dispatches_add_mode(self): + def test_mcp_flag_dispatches_location_set(self): with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: result = runner.invoke(app, ["configure", "skills", "--location", "a.b", "--mcp"]) assert result.exit_code == 0, result.output - mock_mcp.assert_called_once_with(["a.b"], mode="add") - - def test_mcp_remove_dispatches_remove_mode(self): - with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: - result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--mcp", "--remove"] - ) - assert result.exit_code == 0, result.output - mock_mcp.assert_called_once_with(["a.b"], mode="remove") - - def test_mcp_replace_dispatches_replace_mode(self): - with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: - result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--mcp", "--replace"] - ) - assert result.exit_code == 0, result.output - mock_mcp.assert_called_once_with(["a.b"], mode="replace") + mock_mcp.assert_called_once_with(["a.b"]) def test_comma_location_yields_multiple_schemas(self): with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: result = runner.invoke(app, ["configure", "skills", "--location", "a.b, c.d", "--mcp"]) assert result.exit_code == 0, result.output - mock_mcp.assert_called_once_with(["a.b", "c.d"], mode="add") + mock_mcp.assert_called_once_with(["a.b", "c.d"]) def test_without_mcp_is_not_implemented_exit_1(self): with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: @@ -377,15 +361,6 @@ def test_without_mcp_is_not_implemented_exit_1(self): assert result.exit_code == 1 mock_mcp.assert_not_called() - def test_remove_and_replace_together_exit_1(self): - with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: - result = runner.invoke( - app, - ["configure", "skills", "--location", "a.b", "--mcp", "--remove", "--replace"], - ) - assert result.exit_code == 1 - mock_mcp.assert_not_called() - def test_three_part_location_exit_1(self): with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: result = runner.invoke(app, ["configure", "skills", "--location", "a.b.c", "--mcp"]) diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 6f06f82..a520afa 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -1743,29 +1743,6 @@ def _find_skills(servers): return [s for s in servers if s.get("kind") == mcp.SKILLS_MCP_KIND] -class TestResolveSkillLocationSet: - def test_add_is_order_preserving_union(self): - assert mcp.resolve_skill_location_set(["a.b"], ["c.d", "a.b"], mode="add") == [ - "a.b", - "c.d", - ] - - def test_add_to_empty(self): - assert mcp.resolve_skill_location_set([], ["a.b"], mode="add") == ["a.b"] - - def test_remove_is_difference(self): - assert mcp.resolve_skill_location_set(["a.b", "c.d"], ["c.d"], mode="remove") == ["a.b"] - - def test_replace_swaps_in_requested(self): - assert mcp.resolve_skill_location_set(["a.b"], ["x.y", "z.w"], mode="replace") == [ - "x.y", - "z.w", - ] - - def test_replace_with_empty_clears(self): - assert mcp.resolve_skill_location_set(["a.b"], [], mode="replace") == [] - - class TestResolveSkillsMcpServers: def test_builds_single_canonical_entry(self): servers = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["main.default"], []) @@ -1837,7 +1814,7 @@ def _skills_state(mcp_servers=None): class TestConfigureSkillsMcpCommand: - def test_add_to_empty_registers_connection(self, monkeypatch): + def test_set_on_empty_registers_connection(self, monkeypatch): saved_states: list[dict] = [] configured: list[tuple[str, str, str, dict]] = [] _stub_location_base(monkeypatch, _skills_state()) @@ -1848,7 +1825,7 @@ def test_add_to_empty_registers_connection(self, monkeypatch): ) monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - assert mcp.configure_skills_mcp_command(["a.b"], mode="add") == 0 + assert mcp.configure_skills_mcp_command(["a.b"]) == 0 skills = _find_skills(saved_states[-1]["mcp_servers"]) assert len(skills) == 1 @@ -1857,54 +1834,29 @@ def test_add_to_empty_registers_connection(self, monkeypatch): # alwaysLoad rides the per-entry apply (Claude-only). assert configured[0][3]["alwaysLoad"] is True - def test_add_second_appends_in_order(self, monkeypatch): - saved_states: list[dict] = [] - prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a"], []) - _stub_location_base(monkeypatch, _skills_state(prior)) - monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) - monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - - assert mcp.configure_skills_mcp_command(["B.b"], mode="add") == 0 - - assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["A.a", "B.b"] - - def test_remove_from_set(self, monkeypatch): + def test_location_replaces_prior_set(self, monkeypatch): saved_states: list[dict] = [] prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a", "B.b"], []) _stub_location_base(monkeypatch, _skills_state(prior)) monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - assert mcp.configure_skills_mcp_command(["B.b"], mode="remove") == 0 - - assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["A.a"] - - def test_replace_set(self, monkeypatch): - saved_states: list[dict] = [] - prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a", "B.b"], []) - _stub_location_base(monkeypatch, _skills_state(prior)) - monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) - monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - - assert mcp.configure_skills_mcp_command(["X.x"], mode="replace") == 0 + assert mcp.configure_skills_mcp_command(["X.x"]) == 0 assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["X.x"] - def test_remove_last_keeps_entry_with_bare_url(self, monkeypatch): + def test_multiple_locations_set_in_order(self, monkeypatch): saved_states: list[dict] = [] prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a"], []) _stub_location_base(monkeypatch, _skills_state(prior)) monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - assert mcp.configure_skills_mcp_command(["A.a"], mode="remove") == 0 + assert mcp.configure_skills_mcp_command(["X.x", "Y.y"]) == 0 - skills = _find_skills(saved_states[-1]["mcp_servers"]) - assert len(skills) == 1 - assert skills[0]["skill_locations"] == [] - assert skills[0]["url"] == f"{WS}/ai-gateway/skills/" + assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["X.x", "Y.y"] - def test_preserves_mcp_service_entries_across_mutations(self, monkeypatch): + def test_preserves_mcp_service_entries_across_set(self, monkeypatch): saved_states: list[dict] = [] service_entry = { "name": "system-ai-github", @@ -1917,7 +1869,7 @@ def test_preserves_mcp_service_entries_across_mutations(self, monkeypatch): monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - assert mcp.configure_skills_mcp_command(["B.b"], mode="add") == 0 + assert mcp.configure_skills_mcp_command(["B.b"]) == 0 names = [s["name"] for s in saved_states[-1]["mcp_servers"]] assert "system-ai-github" in names From 6fa5fe42747aaafbccecdfd1708536a9c54a943f Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 15:39:05 +0000 Subject: [PATCH 7/7] status: add configure-skills hint to footer Surface `ucode configure skills --location . --mcp` alongside the existing configure-mcp/tracing hints so users discover the Skills connection command from `ucode status`. Co-authored-by: Isaac --- src/ucode/cli.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 36afde4..3ede990 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -773,6 +773,10 @@ def status() -> int: print_note( "Use `ucode configure mcp` to add Databricks MCP servers to configured coding tools." ) + print_note( + "Use `ucode configure skills --location . --mcp` to connect Unity " + "Catalog Skills." + ) print_note("Use `ucode configure tracing` to log coding sessions to an MLflow experiment.") print_note("Use `ucode revert` to clear managed configs and restore prior files.") return 0