From 052989a19537641dfce95d6b7d5bbdf720bea007 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 18 Sep 2026 04:26:25 +0000 Subject: [PATCH] Remove the deprecated `ug configure skills` command `ug skills` now covers everything `ug configure skills` did: a bare `ug skills` registers the schema-less connection, and `ug skills add`/`remove` manage MCP scopes and downloads. Drop the command and its now-dead configure_skills_mcp_command helper, repoint the stale references that named it, and remove the tests that covered it. Co-authored-by: Isaac --- src/ucode/cli.py | 77 -------------------------- src/ucode/mcp.py | 26 +++------ tests/test_cli.py | 135 +--------------------------------------------- tests/test_mcp.py | 100 ++-------------------------------- 4 files changed, 13 insertions(+), 325 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 836a5ddd..bccd802e 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -108,7 +108,6 @@ available_mcp_clients, configure_bare_skills_mcp_command, configure_mcp_command, - configure_skills_mcp_command, configure_skills_mcp_picker_command, configured_mcp_clients, list_mcp_command, @@ -3588,82 +3587,6 @@ def configure( raise typer.Exit(130) from None -@configure_app.command("skills") -def configure_skills( - location: Annotated[ - str | None, - typer.Option("--location", help="Comma-separated `.` skill scopes."), - ] = None, - mcp: Annotated[ - bool, - typer.Option("--mcp", help="Mutate the skills MCP connection instead of downloading."), - ] = False, - path: Annotated[ - str | None, - typer.Option( - "--path", - help="(download) Existing absolute dir to download into; defaults to your home dir.", - ), - ] = None, - names: Annotated[ - str | None, - typer.Option( - "--names", - help="(download) Download exactly these comma-separated fully-qualified " - "`..` skills, spanning any number of schemas. Not valid " - "with --mcp or --location.", - ), - ] = None, -) -> None: - """Configure Databricks Skills for your coding tools. - - When ``--location`` is not provided, registers the skills MCP connection with - utility tools only. - - When ``--location`` is provided: with ``--mcp``, sets the connection's scope to - exactly the listed schemas (no download); otherwise, downloads every skill in - each schema to disk (under ``--path``, or your home dir when omitted) and - registers the MCP connection with utility tools only. ``--names`` instead - downloads a named set of fully-qualified skills that may span schemas (and takes - no ``--location``). - """ - try: - install_databricks_cli(minimum=SKILLS_MCP_MIN_DATABRICKS_CLI_VERSION) - # `--names` absent -> None (whole schemas via --location); present (even - # empty) -> the explicit FQN set, so `--names ""` downloads nothing. - selected_skills = ( - None if names is None else {s.strip() for s in names.split(",") if s.strip()} - ) - if mcp and path is not None: - raise RuntimeError("--path is not valid with --mcp.") - if mcp and selected_skills is not None: - raise RuntimeError("--names is not valid with --mcp; it only applies when downloading.") - if selected_skills is not None and location is not None: - raise RuntimeError("--names takes fully-qualified names; drop --location.") - if selected_skills is not None: - invalid = sorted(s for s in selected_skills if not _is_qualified_skill_name(s)) - if invalid: - raise RuntimeError( - "--names entries must be fully-qualified `..` names " - f"(invalid: {', '.join(invalid)})." - ) - configure_selected_skills_download_command(sorted(selected_skills), path) - return - locations = _parse_skill_locations(location) - if path is not None and not locations: - raise RuntimeError("--path only applies when downloading with --location.") - if mcp or not locations: - configure_skills_mcp_command(locations) - else: - configure_location_skills_download_command(locations, path=path) - except (RuntimeError, ValueError) as exc: - print_err(str(exc)) - raise typer.Exit(1) from None - except KeyboardInterrupt: - print_err("Interrupted.") - raise typer.Exit(130) from None - - @app.command("export", rich_help_panel="Manage") def export_cmd( file_path: Annotated[ diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index da0314ad..88420544 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -1636,7 +1636,7 @@ def configure_mcp_command( excluded_sources = exclude_sources or set() original_mcp_servers: list[dict] = list(state.get("mcp_servers") or []) - # Skills connections are managed by `configure skills`, so keep them out of + # Skills connections are managed by the `ug skills` commands, 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] @@ -1782,7 +1782,7 @@ def remove_mcp_command(agents: set[str] | None = None) -> int: """`ucode mcp remove`: interactively unregister configured MCP servers. Shows the servers currently configured (skills connections excluded — they're - owned by `configure skills`) and removes the ones you select. It never adds or + owned by the `ug skills` commands) and removes the ones you select. It never adds or reconfigures anything, and needs no Databricks auth. Without ``agents``, a selected server is removed from every coding tool it's @@ -2461,17 +2461,6 @@ def _update_skills_mcp( return changed or original != working -def configure_skills_mcp_command(locations: list[str]) -> int: - """Set every configured client's skill scope to ``locations``.""" - state = load_state() - workspace, profile, clients = setup_mcp_clients(state, "Skills MCP") - locations_by_client = _skill_locations_by_client_from_state(state) - for client in clients: - locations_by_client[client] = list(locations) - _update_skills_mcp(state, workspace, profile, clients, locations_by_client) - return 0 - - def _skill_mcp_locations(state: dict) -> list[str]: """The skills MCP connection's ``skill_locations``, or ``[]`` if none exists.""" entry = _skills_entry(list(state.get("mcp_servers") or [])) @@ -2506,12 +2495,11 @@ def register_schemaless_skills_connection( def configure_bare_skills_mcp_command() -> bool: """Register the schema-less skills MCP connection for every configured agent. - The simple entrypoint behind a bare ``ug skills`` (replacing ``ug configure skills`` - with no arguments). Re-registers on every run, preserving any client's existing - ``--mcp`` scope, but only the first run prints anything (the setup header and the - connection summary) -- a repeat ``ug skills`` re-registers silently. Returns whether - no skills connection existed beforehand, so the caller can also show first-run - guidance only then. + The simple entrypoint behind a bare ``ug skills``. Re-registers on every run, + preserving any client's existing ``--mcp`` scope, but only the first run prints + anything (the setup header and the connection summary) -- a repeat ``ug skills`` + re-registers silently. Returns whether no skills connection existed beforehand, so + the caller can also show first-run guidance only then. """ state = load_state() first_time = _skills_entry(list(state.get("mcp_servers") or [])) is None diff --git a/tests/test_cli.py b/tests/test_cli.py index 6a20f55d..61a06e2b 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1887,137 +1887,6 @@ def test_labels_cached_fallback_when_live_auth_fails(self): assert freshness == "cached" -class TestConfigureSkillsCommand: - @pytest.fixture(autouse=True) - def _stub_install_cli(self): - with patch("ucode.cli.install_databricks_cli") as mock_install: - yield mock_install - - def test_requires_skills_mcp_cli_floor(self, _stub_install_cli): - from ucode.databricks import SKILLS_MCP_MIN_DATABRICKS_CLI_VERSION - - with patch("ucode.cli.configure_skills_mcp_command"): - result = runner.invoke(app, ["configure", "skills", "--location", "a.b", "--mcp"]) - assert result.exit_code == 0, result.output - _stub_install_cli.assert_called_once_with(minimum=SKILLS_MCP_MIN_DATABRICKS_CLI_VERSION) - - 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"]) - - 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"]) - - def test_default_mode_dispatches_download_with_path(self): - with patch("ucode.cli.configure_location_skills_download_command") as mock_download: - result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--path", "/tmp/skills"] - ) - assert result.exit_code == 0, result.output - mock_download.assert_called_once_with(["a.b"], path="/tmp/skills") - - def test_default_mode_without_path_dispatches_download(self): - with patch("ucode.cli.configure_location_skills_download_command") as mock_download: - result = runner.invoke(app, ["configure", "skills", "--location", "a.b"]) - assert result.exit_code == 0, result.output - mock_download.assert_called_once_with(["a.b"], path=None) - - def test_skill_downloads_fully_qualified_across_schemas(self): - with patch("ucode.cli.configure_selected_skills_download_command") as mock_download: - result = runner.invoke(app, ["configure", "skills", "--names", "a.b.s1, c.d.s2"]) - assert result.exit_code == 0, result.output - mock_download.assert_called_once_with(["a.b.s1", "c.d.s2"], None) - - def test_skill_threads_path_through(self): - with patch("ucode.cli.configure_selected_skills_download_command") as mock_download: - result = runner.invoke( - app, ["configure", "skills", "--names", "a.b.s1", "--path", "/tmp/skills"] - ) - assert result.exit_code == 0, result.output - mock_download.assert_called_once_with(["a.b.s1"], "/tmp/skills") - - def test_skill_with_location_exit_1(self): - with patch("ucode.cli.configure_selected_skills_download_command") as mock_download: - result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--names", "a.b.s1"] - ) - assert result.exit_code == 1 - assert "--names takes fully-qualified names; drop --location" in _strip_ansi(result.output) - mock_download.assert_not_called() - - def test_bare_skill_exit_1(self): - with patch("ucode.cli.configure_selected_skills_download_command") as mock_download: - result = runner.invoke(app, ["configure", "skills", "--names", "my_skill"]) - assert result.exit_code == 1 - assert "must be fully-qualified" in _strip_ansi(result.output) - mock_download.assert_not_called() - - def test_skill_with_mcp_exit_1(self): - with ( - patch("ucode.cli.configure_skills_mcp_command") as mock_mcp, - patch("ucode.cli.configure_selected_skills_download_command") as mock_download, - ): - result = runner.invoke(app, ["configure", "skills", "--mcp", "--names", "a.b.s1"]) - assert result.exit_code == 1 - assert "--names" in _strip_ansi(result.output) - mock_mcp.assert_not_called() - mock_download.assert_not_called() - - def test_path_with_mcp_exit_1(self): - with ( - patch("ucode.cli.configure_skills_mcp_command") as mock_mcp, - patch("ucode.cli.configure_location_skills_download_command") as mock_download, - ): - result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--mcp", "--path", "/tmp/skills"] - ) - assert result.exit_code == 1 - assert "--path" in _strip_ansi(result.output) - mock_mcp.assert_not_called() - mock_download.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"]) - assert result.exit_code == 1 - mock_mcp.assert_not_called() - - def test_malformed_location_exit_1_names_location(self): - 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_mcp.assert_not_called() - - def test_bare_command_registers_schemaless_connection(self): - with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: - result = runner.invoke(app, ["configure", "skills"]) - assert result.exit_code == 0, result.output - mock_mcp.assert_called_once_with([]) - - def test_mcp_without_location_registers_schemaless_connection(self): - with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: - result = runner.invoke(app, ["configure", "skills", "--mcp"]) - assert result.exit_code == 0, result.output - mock_mcp.assert_called_once_with([]) - - def test_path_without_location_exit_1(self): - with ( - patch("ucode.cli.configure_skills_mcp_command") as mock_mcp, - patch("ucode.cli.configure_location_skills_download_command") as mock_download, - ): - result = runner.invoke(app, ["configure", "skills", "--path", "/tmp/skills"]) - assert result.exit_code == 1 - assert "--path" in _strip_ansi(result.output) - mock_mcp.assert_not_called() - mock_download.assert_not_called() - - class TestSkillsEntrypoint: """Bare `ug skills` registers the schema-less MCP connection, then prints help.""" @@ -2064,8 +1933,8 @@ def test_subcommand_skips_entrypoint_configuration(self): class TestSkillsAddCommand: - """`ucode skills add` is the additive sibling of `configure skills`: `--mcp` - unions schemas into the connection scope, the default mode downloads.""" + """`ucode skills add` is purely additive: `--mcp` unions schemas into the + connection scope, the default mode downloads.""" @pytest.fixture(autouse=True) def _stub_install_cli(self): diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 2845ffab..559f9d1d 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -1390,7 +1390,7 @@ def test_replaces_servers_outside_location(self, monkeypatch): ] def test_preserves_skills_connection(self, monkeypatch): - """A skills connection is owned by `configure skills`, so a replace-mode + """A skills connection is owned by the `ug skills` commands, so a replace-mode `--location` run must leave it registered rather than treating it as a removal.""" saved_states: list[dict] = [] removed: list[tuple[str, str]] = [] @@ -1670,7 +1670,7 @@ def fake_prompt(servers): monkeypatch.setattr(mcp, "_prompt_for_mcp_removal", fake_prompt) assert mcp.remove_mcp_command() == 0 - # The skills connection is owned by `configure skills`, so it's never a + # The skills connection is owned by the `ug skills` commands, so it's never a # removal candidate; only the real MCP server is offered. assert offered["names"] == ["system-ai-github"] @@ -2033,98 +2033,6 @@ def _skills_state(mcp_servers=None): return state -class TestConfigureSkillsMcpCommand: - def test_set_on_empty_registers_connection(self, monkeypatch): - saved_states: list[dict] = [] - configured: list[dict] = [] - _stub_location_base(monkeypatch, _skills_state()) - monkeypatch.setattr( - mcp, - "configure_client_mcp_server", - lambda client, name, url, *a, **kw: ( - configured.append( - { - "client": client, - "name": name, - "url": url, - "always_load": kw.get("always_load"), - } - ) - or [] - ), - ) - monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - - assert mcp.configure_skills_mcp_command(["a.b"]) == 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 is passed through the proxy registration (Claude-only hint). - assert configured[0]["always_load"] is True - - def test_location_replaces_prior_set(self, monkeypatch): - saved_states: list[dict] = [] - prior = mcp._resolve_skills_mcp_servers( - WS, ["claude"], _by_client(["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"]) == 0 - - assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["X.x"] - - def test_multiple_locations_set_in_order(self, monkeypatch): - saved_states: list[dict] = [] - prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], _by_client(["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(["X.x", "Y.y"]) == 0 - - assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["X.x", "Y.y"] - - def test_replaces_scope_for_configured_clients_only(self, monkeypatch): - saved_states: list[dict] = [] - prior = mcp._resolve_skills_mcp_servers( - WS, ["claude", "codex"], {"claude": ["claude.old"], "codex": ["codex.kept"]}, [] - ) - _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(["new.default"]) == 0 - - entry = _find_skills(saved_states[-1]["mcp_servers"])[0] - assert mcp.skill_locations_for_client(entry, "claude") == ["new.default"] - assert mcp.skill_locations_for_client(entry, "codex") == ["codex.kept"] - - def test_preserves_mcp_service_entries_across_set(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"], _by_client(["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"]) == 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 - - class TestSkillMcpLocations: def test_reads_locations_off_skills_entry(self): state = _skills_state( @@ -2184,8 +2092,8 @@ def test_drops_duplicate_new_locations(self): class TestAddSkillsCommand: - """`ucode skills add --mcp` unions schemas into the connection scope rather - than replacing it (unlike `configure_skills_mcp_command`).""" + """`ucode skills add --mcp` unions schemas into the connection scope (additive), + keeping any already configured.""" def test_unions_into_existing_scope(self, monkeypatch): state = _skills_state(