From f86b9ab140a32395e6af72981e806fccd2c2d56d Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Wed, 19 Aug 2026 18:15:12 +0000 Subject: [PATCH 1/2] skills: add `ucode skills add` for additive skill configuration `ucode configure skills --mcp` replaces the skills MCP connection's location set; `ucode skills add` is its additive sibling. With `--mcp` it unions the given schemas into `skill_locations` (via `_union_locations`, the scalar analog of `_union_missing`) instead of replacing them; the default download mode reuses the already-additive download path. Requires `--location`. This mirrors `ucode mcp add` under a new `skills` command namespace, part of moving skill/MCP management off `ucode configure`. Co-authored-by: Isaac --- README.md | 18 +++++++++++ src/ucode/cli.py | 72 +++++++++++++++++++++++++++++++++++++++++++ src/ucode/mcp.py | 23 ++++++++++++++ tests/test_cli.py | 78 +++++++++++++++++++++++++++++++++++++++++++++++ tests/test_mcp.py | 46 ++++++++++++++++++++++++++++ 5 files changed, 237 insertions(+) diff --git a/README.md b/README.md index f26ea6b..ae611b5 100644 --- a/README.md +++ b/README.md @@ -210,6 +210,22 @@ ucode configure skills --location main.default,ml.prod --mcp Each run prints the registered server, its URL, the configured agents, and its tools, and reminds you to run `ucode ` (existing agent sessions need a restart before the MCP tools load). +#### Add skill scopes without replacing existing ones + +`ucode configure skills --mcp` **replaces** the connection's location set with your selection. To +**add** to it instead, use `ucode skills add`. It takes the same `--location`, `--mcp`, `--path`, +and `--skill` options, but is additive: with `--mcp` it unions the given schemas into the scope +rather than replacing it, and download mode leaves already-downloaded skills in place. It requires +`--location`. + +```bash +# Add schemas to the skills MCP scope, keeping any already configured. +ucode skills add --location main.default,ml.prod --mcp + +# Download a schema's skills to disk, keeping existing downloads. +ucode skills add --location main.default +``` + ### Managed config for a workspace (admins) Author the coding config your developers pick up automatically, instead of asking each of them to @@ -297,6 +313,8 @@ their next ucode run. | `ucode configure skills --location main.default [--path ]` | Download a schema's skills to disk (under ``, or your home dir) and register a schema-less skills MCP connection | | `ucode configure skills --location main.default --skill my-skill` | Download only the named skill(s) from a schema (comma-separated for several) | | `ucode configure skills --location main.default --mcp` | Expose a schema's skills as MCP tools (override-only) instead of downloading | +| `ucode skills add --location main.default --mcp` | Add schemas to the skills MCP scope, keeping any already configured (additive; never replaces) | +| `ucode skills add --location main.default` | Download a schema's skills to disk without removing existing downloads | | `ucode setup` | Author the managed config's agents and models (workspace admins only) | | `ucode setup mcps` | Add or change the managed config's MCP servers | | `ucode setup skills [--location a.b,c.d]` | Add or change the managed config's skills | diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 910af65..70919e8 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -94,6 +94,7 @@ MCP_CLIENTS, SKILLS_MCP_KIND, add_mcp_command, + add_skills_command, apply_managed_mcp_servers, apply_managed_skills, configure_mcp_command, @@ -1047,6 +1048,8 @@ def revert() -> int: app.add_typer(configure_app, name="configure", help="Configure workspace and tool settings.") mcp_app = typer.Typer(add_completion=False, no_args_is_help=True) app.add_typer(mcp_app, name="mcp", help="MCP servers exposed by ucode.") +skills_app = typer.Typer(add_completion=False, no_args_is_help=True) +app.add_typer(skills_app, name="skills", help="Databricks Skills for your coding tools.") setup_app = typer.Typer(add_completion=False, no_args_is_help=False) app.add_typer( setup_app, @@ -1126,6 +1129,75 @@ def mcp_web_search_cmd() -> None: serve() +@skills_app.command("add") +def skills_add( + location: Annotated[ + str | None, + typer.Option( + "--location", help="Comma-separated `.` skill scopes to add." + ), + ] = None, + mcp: Annotated[ + bool, + typer.Option( + "--mcp", + help="Add the schemas to the skills MCP connection's scope 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, + skill: Annotated[ + str | None, + typer.Option( + "--skill", + help="(download) Download only this comma-separated subset of skills (by " + "securable name, e.g. `my-skill`) from the schema, instead of every skill. " + "Requires a single --location; not valid with --mcp.", + ), + ] = None, +) -> None: + """Add Databricks Skills to your coding tools, keeping any already configured. + + Like `ucode configure skills`, but additive. With ``--mcp``, unions the given + schemas into the skills MCP connection's scope instead of replacing it; + otherwise downloads each schema's skills to disk (under ``--path``, or your home + dir), leaving already-downloaded skills in place. ``--skill`` narrows a download + to a named subset of a single schema's skills. Requires ``--location``. + """ + try: + locations = _parse_skill_locations(location) + # `--skill` absent -> None (whole schema); present (even empty) -> the + # explicit subset, so `--skill ""` downloads nothing. + selected_skills = ( + None if skill is None else {s.strip() for s in skill.split(",") if s.strip()} + ) + if not locations: + raise RuntimeError("--location is required for `ucode skills add`.") + 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("--skill is not valid with --mcp; it only applies when downloading.") + if selected_skills is not None and len(locations) != 1: + raise RuntimeError( + f"--skill requires a single --location (got: {', '.join(locations)})." + ) + if mcp: + add_skills_command(locations) + else: + configure_skills_download_command(locations, path=path, skills=selected_skills) + 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("mcp-proxy", hidden=True) def mcp_proxy_cmd( url: Annotated[ diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 83ca6f4..c2ff469 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -2158,3 +2158,26 @@ def register_schemaless_skills_connection( ``--mcp`` ``skill_locations`` and otherwise registers the bare schema-less route (utility tools only).""" _update_skills_mcp(state, workspace, profile, clients, _skill_mcp_locations(state)) + + +def _union_locations(base: list[str], new: list[str]) -> list[str]: + """Return ``base`` followed by every location in ``new`` not already present. + The scalar analog of ``_union_missing`` for the skills connection's + ``skill_locations``, so ``ucode skills add --mcp`` grows the scope instead of + replacing it.""" + have = set(base) + return [*base, *[loc for loc in new if loc not in have]] + + +def add_skills_command(locations: list[str]) -> int: + """`ucode skills add --mcp`: union ``locations`` into the skills MCP + connection's ``skill_locations`` WITHOUT dropping any already configured. + + The additive sibling of ``configure_skills_mcp_command`` (which replaces the + scope), mirroring how ``add_mcp_command`` registers servers without removing + the rest.""" + state = load_state() + workspace, profile, clients = setup_mcp_clients(state, "Add Skills MCP") + merged = _union_locations(_skill_mcp_locations(state), locations) + _update_skills_mcp(state, workspace, profile, clients, merged) + return 0 diff --git a/tests/test_cli.py b/tests/test_cli.py index ad45a9a..d1a9772 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -681,6 +681,84 @@ def test_path_without_location_exit_1(self): mock_download.assert_not_called() +class TestSkillsAddCommand: + """`ucode skills add` is the additive sibling of `configure skills`: `--mcp` + unions schemas into the connection scope, the default mode downloads.""" + + def test_mcp_flag_unions_locations(self): + with patch("ucode.cli.add_skills_command") as mock_add: + result = runner.invoke(app, ["skills", "add", "--location", "a.b", "--mcp"]) + assert result.exit_code == 0, result.output + mock_add.assert_called_once_with(["a.b"]) + + def test_comma_location_yields_multiple_schemas(self): + with patch("ucode.cli.add_skills_command") as mock_add: + result = runner.invoke(app, ["skills", "add", "--location", "a.b, c.d", "--mcp"]) + assert result.exit_code == 0, result.output + mock_add.assert_called_once_with(["a.b", "c.d"]) + + def test_default_mode_dispatches_download(self): + with patch("ucode.cli.configure_skills_download_command") as mock_download: + result = runner.invoke(app, ["skills", "add", "--location", "a.b", "--path", "/tmp/s"]) + assert result.exit_code == 0, result.output + mock_download.assert_called_once_with(["a.b"], path="/tmp/s", skills=None) + + def test_skill_filter_dispatches_download_with_subset(self): + with patch("ucode.cli.configure_skills_download_command") as mock_download: + result = runner.invoke(app, ["skills", "add", "--location", "a.b", "--skill", "s1, s2"]) + assert result.exit_code == 0, result.output + mock_download.assert_called_once_with(["a.b"], path=None, skills={"s1", "s2"}) + + def test_without_location_exit_1(self): + with ( + patch("ucode.cli.add_skills_command") as mock_add, + patch("ucode.cli.configure_skills_download_command") as mock_download, + ): + result = runner.invoke(app, ["skills", "add"]) + assert result.exit_code == 1 + assert "--location is required" in _strip_ansi(result.output) + mock_add.assert_not_called() + mock_download.assert_not_called() + + def test_skill_with_mcp_exit_1(self): + with ( + patch("ucode.cli.add_skills_command") as mock_add, + patch("ucode.cli.configure_skills_download_command") as mock_download, + ): + result = runner.invoke( + app, ["skills", "add", "--location", "a.b", "--mcp", "--skill", "s1"] + ) + assert result.exit_code == 1 + assert "--skill" in _strip_ansi(result.output) + mock_add.assert_not_called() + mock_download.assert_not_called() + + def test_path_with_mcp_exit_1(self): + with patch("ucode.cli.add_skills_command") as mock_add: + result = runner.invoke( + app, ["skills", "add", "--location", "a.b", "--mcp", "--path", "/tmp/s"] + ) + assert result.exit_code == 1 + assert "--path" in _strip_ansi(result.output) + mock_add.assert_not_called() + + def test_skill_with_multiple_locations_exit_1(self): + with patch("ucode.cli.configure_skills_download_command") as mock_download: + result = runner.invoke( + app, ["skills", "add", "--location", "a.b, c.d", "--skill", "s1"] + ) + assert result.exit_code == 1 + assert "--skill requires a single --location" in _strip_ansi(result.output) + mock_download.assert_not_called() + + def test_malformed_location_exit_1(self): + with patch("ucode.cli.add_skills_command") as mock_add: + result = runner.invoke(app, ["skills", "add", "--location", "a.b.c", "--mcp"]) + assert result.exit_code == 1 + assert "--location" in _strip_ansi(result.output) + mock_add.assert_not_called() + + class TestApplyManagedSkills: """The launch path both registers the skills MCP connection and downloads bundles to disk.""" diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 18f4e3e..4572a1e 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -2398,6 +2398,52 @@ def test_empty_when_no_skills_entry(self): assert mcp._skill_mcp_locations(_skills_state()) == [] +class TestUnionLocations: + def test_appends_new_after_existing(self): + assert mcp._union_locations(["a.b"], ["c.d"]) == ["a.b", "c.d"] + + def test_drops_locations_already_present(self): + assert mcp._union_locations(["a.b", "c.d"], ["c.d", "e.f"]) == ["a.b", "c.d", "e.f"] + + def test_empty_base_returns_new(self): + assert mcp._union_locations([], ["a.b", "c.d"]) == ["a.b", "c.d"] + + +class TestAddSkillsCommand: + """`ucode skills add --mcp` unions schemas into the connection scope rather + than replacing it (unlike `configure_skills_mcp_command`).""" + + def test_unions_into_existing_scope(self, monkeypatch): + state = _skills_state(mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a"], [])) + _stub_location_base(monkeypatch, state) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda s: None) + + assert mcp.add_skills_command(["B.b"]) == 0 + + assert _find_skills(state["mcp_servers"])[0]["skill_locations"] == ["A.a", "B.b"] + + def test_existing_schema_leaves_scope_unchanged(self, monkeypatch): + state = _skills_state(mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a", "B.b"], [])) + _stub_location_base(monkeypatch, state) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda s: None) + + assert mcp.add_skills_command(["A.a"]) == 0 + + assert _find_skills(state["mcp_servers"])[0]["skill_locations"] == ["A.a", "B.b"] + + def test_registers_scope_from_empty_state(self, monkeypatch): + state = _skills_state() + _stub_location_base(monkeypatch, state) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda s: None) + + assert mcp.add_skills_command(["A.a"]) == 0 + + assert _find_skills(state["mcp_servers"])[0]["skill_locations"] == ["A.a"] + + class TestRegisterSchemalessSkillsConnection: def _stub(self, monkeypatch): saved_states: list[dict] = [] From 13b7fa1ea93ff4366b61d5b41603f2746c9d47be Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Wed, 19 Aug 2026 19:05:15 +0000 Subject: [PATCH 2/2] skills add: address review comments (docs/wording only) - Drop `ucode configure skills` comparisons from the command docstring and README - Tighten `add_skills_command` to a one-line docstring; drop the self-explanatory `_union_locations` docstring and an inline comment - Reword `--mcp` conflict errors to "--path/--skill is not supported when using --mcp" Co-authored-by: Isaac --- README.md | 8 +++----- src/ucode/cli.py | 15 ++++++--------- src/ucode/mcp.py | 11 +---------- 3 files changed, 10 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index ae611b5..1201fff 100644 --- a/README.md +++ b/README.md @@ -212,11 +212,9 @@ you to run `ucode ` (existing agent sessions need a restart before the MC #### Add skill scopes without replacing existing ones -`ucode configure skills --mcp` **replaces** the connection's location set with your selection. To -**add** to it instead, use `ucode skills add`. It takes the same `--location`, `--mcp`, `--path`, -and `--skill` options, but is additive: with `--mcp` it unions the given schemas into the scope -rather than replacing it, and download mode leaves already-downloaded skills in place. It requires -`--location`. +`ucode skills add` registers skills additively, keeping anything already configured. It requires +`--location`; with `--mcp` it adds the schemas to the connection's scope, otherwise it downloads +their skills to disk. ```bash # Add schemas to the skills MCP scope, keeping any already configured. diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 70919e8..923af3b 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -1163,25 +1163,22 @@ def skills_add( ) -> None: """Add Databricks Skills to your coding tools, keeping any already configured. - Like `ucode configure skills`, but additive. With ``--mcp``, unions the given - schemas into the skills MCP connection's scope instead of replacing it; - otherwise downloads each schema's skills to disk (under ``--path``, or your home - dir), leaving already-downloaded skills in place. ``--skill`` narrows a download - to a named subset of a single schema's skills. Requires ``--location``. + With ``--mcp``, adds the given schemas to the skills MCP connection's scope. + Otherwise downloads each schema's skills to disk (under ``--path``, or your home + dir), keeping already-downloaded skills. ``--skill`` narrows a download to a + named subset of a single schema's skills. Requires ``--location``. """ try: locations = _parse_skill_locations(location) - # `--skill` absent -> None (whole schema); present (even empty) -> the - # explicit subset, so `--skill ""` downloads nothing. selected_skills = ( None if skill is None else {s.strip() for s in skill.split(",") if s.strip()} ) if not locations: raise RuntimeError("--location is required for `ucode skills add`.") if mcp and path is not None: - raise RuntimeError("--path is not valid with --mcp.") + raise RuntimeError("--path is not supported when using --mcp") if mcp and selected_skills is not None: - raise RuntimeError("--skill is not valid with --mcp; it only applies when downloading.") + raise RuntimeError("--skill is not supported when using --mcp") if selected_skills is not None and len(locations) != 1: raise RuntimeError( f"--skill requires a single --location (got: {', '.join(locations)})." diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index c2ff469..b57d58d 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -2161,21 +2161,12 @@ def register_schemaless_skills_connection( def _union_locations(base: list[str], new: list[str]) -> list[str]: - """Return ``base`` followed by every location in ``new`` not already present. - The scalar analog of ``_union_missing`` for the skills connection's - ``skill_locations``, so ``ucode skills add --mcp`` grows the scope instead of - replacing it.""" have = set(base) return [*base, *[loc for loc in new if loc not in have]] def add_skills_command(locations: list[str]) -> int: - """`ucode skills add --mcp`: union ``locations`` into the skills MCP - connection's ``skill_locations`` WITHOUT dropping any already configured. - - The additive sibling of ``configure_skills_mcp_command`` (which replaces the - scope), mirroring how ``add_mcp_command`` registers servers without removing - the rest.""" + """Add ``locations`` to the skills MCP connection's scope, keeping any already configured.""" state = load_state() workspace, profile, clients = setup_mcp_clients(state, "Add Skills MCP") merged = _union_locations(_skill_mcp_locations(state), locations)