diff --git a/AGENTS.md b/AGENTS.md index 7810cb9..6a2872b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,7 +20,7 @@ Tests live in `tests/`. - Use Python 3.12+. - Keep changes scoped to the requested behavior. -- Follow the existing module boundaries: CLI orchestration in `cli.py`, agent-specific behavior in `agents/.py`, shared agent dispatch in `agents/__init__.py`, Databricks calls in `databricks.py`, skill download (UC fetch client + on-disk writer + download orchestration) in `skills_download.py`, MCP-connection state glue in `mcp.py`, and presentation helpers in `ui.py`. Skill download persists no disk state — it writes files to `--path` (or the home dir) and registers only the schema-less skills MCP connection. +- Follow the existing module boundaries: CLI orchestration in `cli.py`, agent-specific behavior in `agents/.py`, shared agent dispatch in `agents/__init__.py`, Databricks calls in `databricks.py`, skill download (UC fetch client + on-disk writer + download orchestration) in `skills_download.py`, MCP-connection state glue in `mcp.py`, and presentation helpers in `ui.py`. Skill download persists no disk state — it writes files to `--path` (or the home dir) and registers only the schema-less skills MCP connection. `ucode configure skills` with no `--location` (or `--mcp` with no `--location`) registers that schema-less connection without downloading anything. - Prefer existing helpers for config file writes, state persistence, UI messages, and Databricks authentication. - Add or update focused tests for behavior changes. - Do not modify generated or lock files unless the dependency graph intentionally changes. diff --git a/README.md b/README.md index 8e5f4b6..b856249 100644 --- a/README.md +++ b/README.md @@ -115,25 +115,32 @@ then registers the servers); pass a comma-separated list to register several at ### Skills (optional) -Configure Unity Catalog Skills for your coding tools with `ucode configure skills`. It has two -mutually-exclusive modes, both scoped by `--location .` (comma-separated for -multiple schemas): +Configure Unity Catalog Skills for your coding tools with `ucode configure skills`: ```bash -# Download mode (default): fetch every skill in the schema to disk. +# Utility tools only: register the schema-less skills MCP connection, no download. +ucode configure skills + +# Download mode: fetch every skill in the schema to disk (and register the connection). ucode configure skills --location main.default --path /abs/project/dir # MCP mode: expose the schema's skills as MCP tools instead of downloading. ucode configure skills --location main.default,ml.prod --mcp ``` -- **Download mode** writes each skill flat as `/SKILL.md` (plus its bundled files) into both - `.claude/skills/` and `.agents/skills/`. `--path` (an existing absolute directory) is optional; - when omitted, skills are written under your home directory. Any pre-existing skill dir prompts - before it's overwritten. It then registers a schema-less skills MCP connection (utility tools - only), leaving any prior `--mcp` scope untouched. -- **MCP mode** sets the connection's location set to exactly `` (override-only) and rebuilds - its `?schema=` URL; no files are downloaded and `--path` is rejected. +- **Bare command** (no `--location`) registers the schema-less skills MCP connection — the + cross-schema utility tools only — and downloads nothing. `--mcp` with no `--location` does the + same. +- **Download mode** (with `--location`, no `--mcp`) writes each skill flat as `/SKILL.md` + (plus its bundled files) into both `.claude/skills/` and `.agents/skills/`. `--path` (an existing + absolute directory) is optional; when omitted, skills are written under your home directory. Any + pre-existing skill dir prompts before it's overwritten. It then registers a schema-less skills + MCP connection, leaving any prior `--mcp` scope untouched. +- **MCP mode** (`--location … --mcp`) sets the connection's location set to exactly `` + (override-only) and rebuilds its `?schema=` URL; no files are downloaded and `--path` is rejected. + +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). --- @@ -151,6 +158,7 @@ ucode configure skills --location main.default,ml.prod --mcp | `ucode configure --profiles DEFAULT --use-pat` | Authenticate with the profile's personal access token — no browser login | | `ucode configure --skip-validate` | Write configs without sending a test message through each agent | | `ucode configure --agents claude --mcp system.ai.slack` | Configure an agent and register its Databricks MCP server(s) in one command | +| `ucode configure skills` | Register the skills MCP connection (utility tools only); no skills download | | `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 --mcp` | Expose a schema's skills as MCP tools (override-only) instead of downloading | diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 1a5b197..e5bab4e 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -148,11 +148,12 @@ def _parse_agents_option(agents: str) -> list[str]: return tools -def _parse_skill_locations(location: str) -> list[str]: +def _parse_skill_locations(location: str | None) -> list[str]: """Parse a comma-separated `--location` into `.` refs, - dropping duplicates while preserving order.""" + dropping duplicates while preserving order. `None`/empty yields `[]` (the + schema-less, utility-tools-only connection).""" locations: list[str] = [] - for raw in location.split(","): + for raw in (location or "").split(","): raw = raw.strip() if not raw: continue @@ -161,11 +162,6 @@ def _parse_skill_locations(location: str) -> list[str]: raise RuntimeError(f"--location entries must be `.`, got `{raw}`.") if raw not in locations: locations.append(raw) - if not locations: - raise RuntimeError( - "No schemas provided for --location. Use `.`, " - "comma-separated for multiple." - ) return locations @@ -803,8 +799,7 @@ def status() -> int: "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." + "Use `ucode configure skills` to set up Unity Catalog Skills for configured coding tools." ) 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.") @@ -1522,9 +1517,9 @@ def configure_mcp( @configure_app.command("skills") def configure_skills( location: Annotated[ - str, + 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."), @@ -1539,18 +1534,24 @@ def configure_skills( ) -> None: """Configure Databricks Skills for your coding tools. - By default, downloads every skill in each ``--location`` schema to disk - (under ``--path``, or your home dir when omitted) and registers a schema-less - MCP connection. With ``--mcp``, instead sets the skills MCP connection's scope - to exactly the listed schemas. + 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. """ try: - if mcp: - if path is not None: - raise RuntimeError("--path is not valid with --mcp.") - configure_skills_mcp_command(_parse_skill_locations(location)) + locations = _parse_skill_locations(location) + if mcp and path is not None: + raise RuntimeError("--path is not valid with --mcp.") + 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_skills_download_command(_parse_skill_locations(location), path=path) + configure_skills_download_command(locations, path=path) except (RuntimeError, ValueError) 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 cdbe4a4..1d5d7b7 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -43,6 +43,8 @@ ) from ucode.state import load_full_state, load_state, save_state from ucode.ui import ( + console, + print_kv, print_note, print_section, print_success, @@ -1460,6 +1462,37 @@ def _resolve_skills_mcp_servers( return [*kept, _build_skills_entry(workspace, locations, merged)] +def _join_with_and(items: list[str]) -> str: + if len(items) <= 1: + return items[0] if items else "" + return ", ".join(items[:-1]) + " and " + items[-1] + + +def _skills_tools_description(locations: list[str]) -> str: + if not locations: + return "UC skill utility tools" + return f"UC skill utility tools + skills tools in schema {_join_with_and(locations)}" + + +def _print_skills_summary(entry: dict) -> None: + """Report the registered skills connection and how to start using it.""" + clients = [ + str(MCP_CLIENTS[client]["display"]) + for client in (entry.get("clients") or []) + if client in MCP_CLIENTS + ] + console.print() + print_success("Skills MCP registered") + print_kv("Server", str(entry.get("name") or SKILLS_MCP_SERVER_NAME)) + print_kv("URL", str(entry.get("url") or "")) + print_kv("Configured", ", ".join(clients) if clients else "none") + print_kv("Tools", _skills_tools_description(entry.get("skill_locations") or [])) + print_note( + "Run `ucode ` to use the skills MCP. For existing sessions, " + "restart the agent for the skills to take effect." + ) + + def _update_skills_mcp( state: dict, workspace: str, profile: str | None, clients: list[str], locations: list[str] ) -> None: @@ -1470,7 +1503,8 @@ def _update_skills_mcp( if changed or original != working: state["mcp_servers"] = working save_state(state) - print_success("Saved") + entry = next(s for s in working if s.get("kind") == SKILLS_MCP_KIND) + _print_skills_summary(entry) def configure_skills_mcp_command(locations: list[str]) -> int: diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index d0bc9a4..8822640 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -15,7 +15,14 @@ ) from ucode.mcp import register_schemaless_skills_connection, setup_mcp_clients from ucode.state import load_state -from ucode.ui import print_note, print_success, print_warning, progress_bar, prompt_yes_no +from ucode.ui import ( + console, + print_note, + print_success, + print_warning, + progress_bar, + prompt_yes_no, +) # `.claude/skills` (Claude) + `.agents/skills` (the alias other agents read). SKILL_BASE_DIR_NAMES = (".claude/skills", ".agents/skills") @@ -248,6 +255,7 @@ def download_skills(workspace: str, token: str, locations: list[str], path: str skill warns and skips it without aborting the batch. """ roots = skill_dir_roots(path) + roots_display = " and ".join(str(root) for root in roots) for location in locations: catalog, schema = location.split(".") leaves, reason = list_schema_skills(workspace, token, catalog, schema) @@ -267,7 +275,10 @@ def download_skills(workspace: str, token: str, locations: list[str], path: str continue if write_skill(roots, leaf, files, location=location): written += 1 - print_success(f"Downloaded {written}/{len(leaves)} skill(s) from `{location}`.") + console.print() + print_success( + f"Downloaded {written}/{len(leaves)} skill(s) from `{location}` in {roots_display}." + ) def configure_skills_download_command(locations: list[str], *, path: str | None) -> int: diff --git a/tests/test_cli.py b/tests/test_cli.py index 374041d..cb1d128 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -441,9 +441,28 @@ def test_malformed_location_exit_1_names_location(self): assert "--location" in _strip_ansi(result.output) mock_mcp.assert_not_called() - def test_missing_location_is_typer_usage_error(self): - result = runner.invoke(app, ["configure", "skills"]) - assert result.exit_code == 2 + 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_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 TestStatusSkillsSection: diff --git a/tests/test_mcp.py b/tests/test_mcp.py index c143a7a..1600096 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -19,6 +19,11 @@ PROXY_TAIL = ["mcp-proxy", "--url", GH_URL, "--host", WS, "--profile", "p"] +def _unwrap(text: str) -> str: + """Collapse rich's line-wrapping so assertions match regardless of terminal width.""" + return " ".join(text.split()) + + def _proxy_argv() -> list[str]: from ucode.databricks import build_mcp_proxy_argv @@ -1950,6 +1955,50 @@ def test_preserves_prior_mcp_location_set(self, monkeypatch): assert _find_skills(state["mcp_servers"])[0]["skill_locations"] == ["X.x", "Y.y"] +class TestSkillsToolsDescription: + def test_bare_route_names_utility_tools_only(self): + assert mcp._skills_tools_description([]) == "UC skill utility tools" + + def test_scoped_names_utility_plus_skills_tools(self): + assert mcp._skills_tools_description(["main.default"]) == ( + "UC skill utility tools + skills tools in schema main.default" + ) + + def test_multiple_schemas_joined_with_and(self): + assert mcp._skills_tools_description(["a.b", "c.d", "e.f"]) == ( + "UC skill utility tools + skills tools in schema a.b, c.d and e.f" + ) + + +class TestPrintSkillsSummary: + def _entry(self, locations): + return mcp._resolve_skills_mcp_servers(WS, ["claude", "codex"], locations, [])[0] + + def test_reports_scoped_connection(self, capsys): + mcp._print_skills_summary(self._entry(["main.default"])) + assert _unwrap(capsys.readouterr().out) == ( + "✔ Skills MCP registered " + "Server: databricks-skill-registry " + f"URL: {WS}/ai-gateway/skills/?schema=main.default " + "Configured: Claude Code, Codex " + "Tools: UC skill utility tools + skills tools in schema main.default " + "• Run `ucode ` to use the skills MCP. For existing sessions, " + "restart the agent for the skills to take effect." + ) + + def test_reports_schemaless_connection(self, capsys): + mcp._print_skills_summary(self._entry([])) + assert _unwrap(capsys.readouterr().out) == ( + "✔ Skills MCP registered " + "Server: databricks-skill-registry " + f"URL: {WS}/ai-gateway/skills/ " + "Configured: Claude Code, Codex " + "Tools: UC skill utility tools " + "• Run `ucode ` to use the skills MCP. For existing sessions, " + "restart the agent for the skills to take effect." + ) + + class TestRevertMcpConfigs: def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkeypatch): removed: list[tuple[str, str]] = [] diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index 827bdae..33ccf62 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -337,13 +337,17 @@ def test_bundle_failure_skips_that_skill_only(self, tmp_path, monkeypatch): assert (tmp_path / ".claude/skills/good/SKILL.md").read_bytes() == b"ok" assert not (tmp_path / ".claude/skills/bad").exists() - def test_prints_downloaded_count_summary(self, tmp_path, monkeypatch, capsys): + def test_prints_downloaded_count_and_roots_summary(self, tmp_path, monkeypatch, capsys): monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["a", "b", "c"], None)) monkeypatch.setattr(sd, "fetch_skill_bundle", lambda *a, **k: ({"SKILL.md": b"x"}, None)) sd.download_skills(WS, "token", ["main.default"], str(tmp_path)) - assert "Downloaded 3/3 skill(s) from `main.default`" in capsys.readouterr().out + # Rich wraps long paths across lines; strip all whitespace from both sides to compare. + roots = sd.skill_dir_roots(str(tmp_path)) + expected = f"Downloaded 3/3 skill(s) from `main.default` in {roots[0]} and {roots[1]}." + printed = "".join(capsys.readouterr().out.split()) + assert "".join(expected.split()) in printed def test_summary_counts_only_written_skills(self, tmp_path, monkeypatch, capsys): monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["good", "bad"], None)) @@ -357,7 +361,7 @@ def test_summary_counts_only_written_skills(self, tmp_path, monkeypatch, capsys) sd.download_skills(WS, "token", ["main.default"], str(tmp_path)) - assert "Downloaded 1/2 skill(s) from `main.default`" in capsys.readouterr().out + assert "Downloaded 1/2 skill(s) from `main.default` in" in capsys.readouterr().out class TestConfigureSkillsDownloadCommand: @@ -394,3 +398,4 @@ def test_none_path_threads_through(self, monkeypatch): assert sd.configure_skills_download_command(["a.b"], path=None) == 0 assert calls["download"] == (WS, "token", ["a.b"], None) + assert calls["register"] == (WS, "profile", ["claude"])