Skip to content

Commit 4740381

Browse files
authored
configure skills: utility-tools-only mode, announce registration, restart hint (#248)
* configure skills: utility-tools-only mode, announce registration, restart hint Addresses three bug-bash P0 gaps in `ucode configure skills`: - Make `--location` optional. A bare `ucode configure skills` (or `--mcp` with no location) now registers the schema-less skills MCP connection — the cross-schema utility tools only — without forcing a download. `--path` without `--location` is rejected. - Announce the registration: print the server name, URL, configured agents, and a tool-category one-liner (utility tools, plus live per-schema skill tools when scoped). Download mode also prints where files landed. The summary names categories, not individual tools, so it can't drift when the backend changes its offering. - Always end with a launch/restart line ("Run `ucode <agent>` ...; existing sessions need a restart"), and note that downloaded files already work from disk. The summary prints even on a no-op re-run; only the save is gated on a real change. Co-authored-by: Isaac * Address review comments - mcp.py: drop the download-specific restart clause; rename `_join_human` to `_join_with_and`; tighten `_print_skills_summary` docstring and restore the original `_update_skills_mcp` docstring; rename the local `displays` to `clients`; remove the now-unused `download_roots` plumbing and `Path` import. - Drop "live" from the tools summary line. - skills_download.py: fold the download roots into the per-location summary ("Downloaded N/M skill(s) from `x.y` in <roots>") instead of a separate note. - README / status hint: reword the skills copy. - Tests: assert the full summary strings (not substrings); update the download summary + dispatch tests accordingly. Co-authored-by: Isaac * Address review comments: rename tools helper, add status markers - Rename `_skills_tools_summary` to `_skills_tools_description` and drop its docstring (the function is self-explanatory). - Prefix the summary heading with ✔ and the restart note with ⚠. Co-authored-by: Isaac * Drop the ⚠ marker from the restart note Keep the plain `print_note` bullet for the restart line; the ✔ heading stays. Co-authored-by: Isaac * Drop the redundant "Saved" line from the skills summary The `✔ Skills MCP registered` summary heading now serves as the confirmation, so the separate `✔ Saved` line was printing two checkmarks for one action. Co-authored-by: Isaac * Address review comments: reword skills docstring and restart note - Restructure the `configure_skills` docstring around whether `--location` is given, then whether `--mcp` is present. - Reword the restart note to "restart the agent for the skills to take effect". Co-authored-by: Isaac * Clarify download-path docstring wording The download path registers the MCP connection with utility tools only. Co-authored-by: Isaac * Match the summary checkmark color and space the download line - Color the summary heading's ✔ green so it matches the green ✔ from `print_success` on the download line (the heading's bold-wrap was rendering it white). - Print a blank line before each per-location "Downloaded ..." line. Co-authored-by: Isaac * Use print_success for the skills summary heading Replace the manually-greened print_heading with print_success so the summary heading's ✔ matches the download line's checkmark (both green, no bold-wrap or inline color markup). Keep the leading blank line for spacing. Co-authored-by: Isaac
1 parent 54bdc27 commit 4740381

8 files changed

Lines changed: 169 additions & 42 deletions

File tree

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ Tests live in `tests/`.
2020

2121
- Use Python 3.12+.
2222
- Keep changes scoped to the requested behavior.
23-
- Follow the existing module boundaries: CLI orchestration in `cli.py`, agent-specific behavior in `agents/<name>.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.
23+
- Follow the existing module boundaries: CLI orchestration in `cli.py`, agent-specific behavior in `agents/<name>.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.
2424
- Prefer existing helpers for config file writes, state persistence, UI messages, and Databricks authentication.
2525
- Add or update focused tests for behavior changes.
2626
- Do not modify generated or lock files unless the dependency graph intentionally changes.

README.md

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -115,25 +115,32 @@ then registers the servers); pass a comma-separated list to register several at
115115

116116
### Skills (optional)
117117

118-
Configure Unity Catalog Skills for your coding tools with `ucode configure skills`. It has two
119-
mutually-exclusive modes, both scoped by `--location <catalog>.<schema>` (comma-separated for
120-
multiple schemas):
118+
Configure Unity Catalog Skills for your coding tools with `ucode configure skills`:
121119

122120
```bash
123-
# Download mode (default): fetch every skill in the schema to disk.
121+
# Utility tools only: register the schema-less skills MCP connection, no download.
122+
ucode configure skills
123+
124+
# Download mode: fetch every skill in the schema to disk (and register the connection).
124125
ucode configure skills --location main.default --path /abs/project/dir
125126

126127
# MCP mode: expose the schema's skills as MCP tools instead of downloading.
127128
ucode configure skills --location main.default,ml.prod --mcp
128129
```
129130

130-
- **Download mode** writes each skill flat as `<leaf>/SKILL.md` (plus its bundled files) into both
131-
`.claude/skills/` and `.agents/skills/`. `--path` (an existing absolute directory) is optional;
132-
when omitted, skills are written under your home directory. Any pre-existing skill dir prompts
133-
before it's overwritten. It then registers a schema-less skills MCP connection (utility tools
134-
only), leaving any prior `--mcp` scope untouched.
135-
- **MCP mode** sets the connection's location set to exactly `<list>` (override-only) and rebuilds
136-
its `?schema=` URL; no files are downloaded and `--path` is rejected.
131+
- **Bare command** (no `--location`) registers the schema-less skills MCP connection — the
132+
cross-schema utility tools only — and downloads nothing. `--mcp` with no `--location` does the
133+
same.
134+
- **Download mode** (with `--location`, no `--mcp`) writes each skill flat as `<leaf>/SKILL.md`
135+
(plus its bundled files) into both `.claude/skills/` and `.agents/skills/`. `--path` (an existing
136+
absolute directory) is optional; when omitted, skills are written under your home directory. Any
137+
pre-existing skill dir prompts before it's overwritten. It then registers a schema-less skills
138+
MCP connection, leaving any prior `--mcp` scope untouched.
139+
- **MCP mode** (`--location … --mcp`) sets the connection's location set to exactly `<list>`
140+
(override-only) and rebuilds its `?schema=` URL; no files are downloaded and `--path` is rejected.
141+
142+
Each run prints the registered server, its URL, the configured agents, and its tools, and reminds
143+
you to run `ucode <agent>` (existing agent sessions need a restart before the MCP tools load).
137144

138145
---
139146

@@ -151,6 +158,7 @@ ucode configure skills --location main.default,ml.prod --mcp
151158
| `ucode configure --profiles DEFAULT --use-pat` | Authenticate with the profile's personal access token — no browser login |
152159
| `ucode configure --skip-validate` | Write configs without sending a test message through each agent |
153160
| `ucode configure --agents claude --mcp system.ai.slack` | Configure an agent and register its Databricks MCP server(s) in one command |
161+
| `ucode configure skills` | Register the skills MCP connection (utility tools only); no skills download |
154162
| `ucode configure skills --location main.default [--path <dir>]` | Download a schema's skills to disk (under `<dir>`, or your home dir) and register a schema-less skills MCP connection |
155163
| `ucode configure skills --location main.default --mcp` | Expose a schema's skills as MCP tools (override-only) instead of downloading |
156164

src/ucode/cli.py

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -148,11 +148,12 @@ def _parse_agents_option(agents: str) -> list[str]:
148148
return tools
149149

150150

151-
def _parse_skill_locations(location: str) -> list[str]:
151+
def _parse_skill_locations(location: str | None) -> list[str]:
152152
"""Parse a comma-separated `--location` into `<catalog>.<schema>` refs,
153-
dropping duplicates while preserving order."""
153+
dropping duplicates while preserving order. `None`/empty yields `[]` (the
154+
schema-less, utility-tools-only connection)."""
154155
locations: list[str] = []
155-
for raw in location.split(","):
156+
for raw in (location or "").split(","):
156157
raw = raw.strip()
157158
if not raw:
158159
continue
@@ -161,11 +162,6 @@ def _parse_skill_locations(location: str) -> list[str]:
161162
raise RuntimeError(f"--location entries must be `<catalog>.<schema>`, got `{raw}`.")
162163
if raw not in locations:
163164
locations.append(raw)
164-
if not locations:
165-
raise RuntimeError(
166-
"No schemas provided for --location. Use `<catalog>.<schema>`, "
167-
"comma-separated for multiple."
168-
)
169165
return locations
170166

171167

@@ -803,8 +799,7 @@ def status() -> int:
803799
"Use `ucode configure mcp` to add Databricks MCP servers to configured coding tools."
804800
)
805801
print_note(
806-
"Use `ucode configure skills --location <catalog>.<schema> --mcp` to connect Unity "
807-
"Catalog Skills."
802+
"Use `ucode configure skills` to set up Unity Catalog Skills for configured coding tools."
808803
)
809804
print_note("Use `ucode configure tracing` to log coding sessions to an MLflow experiment.")
810805
print_note("Use `ucode revert` to clear managed configs and restore prior files.")
@@ -1522,9 +1517,9 @@ def configure_mcp(
15221517
@configure_app.command("skills")
15231518
def configure_skills(
15241519
location: Annotated[
1525-
str,
1520+
str | None,
15261521
typer.Option("--location", help="Comma-separated `<catalog>.<schema>` skill scopes."),
1527-
],
1522+
] = None,
15281523
mcp: Annotated[
15291524
bool,
15301525
typer.Option("--mcp", help="Mutate the skills MCP connection instead of downloading."),
@@ -1539,18 +1534,24 @@ def configure_skills(
15391534
) -> None:
15401535
"""Configure Databricks Skills for your coding tools.
15411536
1542-
By default, downloads every skill in each ``--location`` schema to disk
1543-
(under ``--path``, or your home dir when omitted) and registers a schema-less
1544-
MCP connection. With ``--mcp``, instead sets the skills MCP connection's scope
1545-
to exactly the listed schemas.
1537+
When ``--location`` is not provided, registers the skills MCP connection with
1538+
utility tools only.
1539+
1540+
When ``--location`` is provided: with ``--mcp``, sets the connection's scope to
1541+
exactly the listed schemas (no download); otherwise, downloads every skill in
1542+
each schema to disk (under ``--path``, or your home dir when omitted) and
1543+
registers the MCP connection with utility tools only.
15461544
"""
15471545
try:
1548-
if mcp:
1549-
if path is not None:
1550-
raise RuntimeError("--path is not valid with --mcp.")
1551-
configure_skills_mcp_command(_parse_skill_locations(location))
1546+
locations = _parse_skill_locations(location)
1547+
if mcp and path is not None:
1548+
raise RuntimeError("--path is not valid with --mcp.")
1549+
if path is not None and not locations:
1550+
raise RuntimeError("--path only applies when downloading with --location.")
1551+
if mcp or not locations:
1552+
configure_skills_mcp_command(locations)
15521553
else:
1553-
configure_skills_download_command(_parse_skill_locations(location), path=path)
1554+
configure_skills_download_command(locations, path=path)
15541555
except (RuntimeError, ValueError) as exc:
15551556
print_err(str(exc))
15561557
raise typer.Exit(1) from None

src/ucode/mcp.py

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@
4343
)
4444
from ucode.state import load_full_state, load_state, save_state
4545
from ucode.ui import (
46+
console,
47+
print_kv,
4648
print_note,
4749
print_section,
4850
print_success,
@@ -1460,6 +1462,37 @@ def _resolve_skills_mcp_servers(
14601462
return [*kept, _build_skills_entry(workspace, locations, merged)]
14611463

14621464

1465+
def _join_with_and(items: list[str]) -> str:
1466+
if len(items) <= 1:
1467+
return items[0] if items else ""
1468+
return ", ".join(items[:-1]) + " and " + items[-1]
1469+
1470+
1471+
def _skills_tools_description(locations: list[str]) -> str:
1472+
if not locations:
1473+
return "UC skill utility tools"
1474+
return f"UC skill utility tools + skills tools in schema {_join_with_and(locations)}"
1475+
1476+
1477+
def _print_skills_summary(entry: dict) -> None:
1478+
"""Report the registered skills connection and how to start using it."""
1479+
clients = [
1480+
str(MCP_CLIENTS[client]["display"])
1481+
for client in (entry.get("clients") or [])
1482+
if client in MCP_CLIENTS
1483+
]
1484+
console.print()
1485+
print_success("Skills MCP registered")
1486+
print_kv("Server", str(entry.get("name") or SKILLS_MCP_SERVER_NAME))
1487+
print_kv("URL", str(entry.get("url") or ""))
1488+
print_kv("Configured", ", ".join(clients) if clients else "none")
1489+
print_kv("Tools", _skills_tools_description(entry.get("skill_locations") or []))
1490+
print_note(
1491+
"Run `ucode <agent>` to use the skills MCP. For existing sessions, "
1492+
"restart the agent for the skills to take effect."
1493+
)
1494+
1495+
14631496
def _update_skills_mcp(
14641497
state: dict, workspace: str, profile: str | None, clients: list[str], locations: list[str]
14651498
) -> None:
@@ -1470,7 +1503,8 @@ def _update_skills_mcp(
14701503
if changed or original != working:
14711504
state["mcp_servers"] = working
14721505
save_state(state)
1473-
print_success("Saved")
1506+
entry = next(s for s in working if s.get("kind") == SKILLS_MCP_KIND)
1507+
_print_skills_summary(entry)
14741508

14751509

14761510
def configure_skills_mcp_command(locations: list[str]) -> int:

src/ucode/skills_download.py

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,14 @@
1515
)
1616
from ucode.mcp import register_schemaless_skills_connection, setup_mcp_clients
1717
from ucode.state import load_state
18-
from ucode.ui import print_note, print_success, print_warning, progress_bar, prompt_yes_no
18+
from ucode.ui import (
19+
console,
20+
print_note,
21+
print_success,
22+
print_warning,
23+
progress_bar,
24+
prompt_yes_no,
25+
)
1926

2027
# `.claude/skills` (Claude) + `.agents/skills` (the alias other agents read).
2128
SKILL_BASE_DIR_NAMES = (".claude/skills", ".agents/skills")
@@ -248,6 +255,7 @@ def download_skills(workspace: str, token: str, locations: list[str], path: str
248255
skill warns and skips it without aborting the batch.
249256
"""
250257
roots = skill_dir_roots(path)
258+
roots_display = " and ".join(str(root) for root in roots)
251259
for location in locations:
252260
catalog, schema = location.split(".")
253261
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
267275
continue
268276
if write_skill(roots, leaf, files, location=location):
269277
written += 1
270-
print_success(f"Downloaded {written}/{len(leaves)} skill(s) from `{location}`.")
278+
console.print()
279+
print_success(
280+
f"Downloaded {written}/{len(leaves)} skill(s) from `{location}` in {roots_display}."
281+
)
271282

272283

273284
def configure_skills_download_command(locations: list[str], *, path: str | None) -> int:

tests/test_cli.py

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -441,9 +441,28 @@ def test_malformed_location_exit_1_names_location(self):
441441
assert "--location" in _strip_ansi(result.output)
442442
mock_mcp.assert_not_called()
443443

444-
def test_missing_location_is_typer_usage_error(self):
445-
result = runner.invoke(app, ["configure", "skills"])
446-
assert result.exit_code == 2
444+
def test_bare_command_registers_schemaless_connection(self):
445+
with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp:
446+
result = runner.invoke(app, ["configure", "skills"])
447+
assert result.exit_code == 0, result.output
448+
mock_mcp.assert_called_once_with([])
449+
450+
def test_mcp_without_location_registers_schemaless_connection(self):
451+
with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp:
452+
result = runner.invoke(app, ["configure", "skills", "--mcp"])
453+
assert result.exit_code == 0, result.output
454+
mock_mcp.assert_called_once_with([])
455+
456+
def test_path_without_location_exit_1(self):
457+
with (
458+
patch("ucode.cli.configure_skills_mcp_command") as mock_mcp,
459+
patch("ucode.cli.configure_skills_download_command") as mock_download,
460+
):
461+
result = runner.invoke(app, ["configure", "skills", "--path", "/tmp/skills"])
462+
assert result.exit_code == 1
463+
assert "--path" in _strip_ansi(result.output)
464+
mock_mcp.assert_not_called()
465+
mock_download.assert_not_called()
447466

448467

449468
class TestStatusSkillsSection:

tests/test_mcp.py

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,11 @@
1919
PROXY_TAIL = ["mcp-proxy", "--url", GH_URL, "--host", WS, "--profile", "p"]
2020

2121

22+
def _unwrap(text: str) -> str:
23+
"""Collapse rich's line-wrapping so assertions match regardless of terminal width."""
24+
return " ".join(text.split())
25+
26+
2227
def _proxy_argv() -> list[str]:
2328
from ucode.databricks import build_mcp_proxy_argv
2429

@@ -1950,6 +1955,50 @@ def test_preserves_prior_mcp_location_set(self, monkeypatch):
19501955
assert _find_skills(state["mcp_servers"])[0]["skill_locations"] == ["X.x", "Y.y"]
19511956

19521957

1958+
class TestSkillsToolsDescription:
1959+
def test_bare_route_names_utility_tools_only(self):
1960+
assert mcp._skills_tools_description([]) == "UC skill utility tools"
1961+
1962+
def test_scoped_names_utility_plus_skills_tools(self):
1963+
assert mcp._skills_tools_description(["main.default"]) == (
1964+
"UC skill utility tools + skills tools in schema main.default"
1965+
)
1966+
1967+
def test_multiple_schemas_joined_with_and(self):
1968+
assert mcp._skills_tools_description(["a.b", "c.d", "e.f"]) == (
1969+
"UC skill utility tools + skills tools in schema a.b, c.d and e.f"
1970+
)
1971+
1972+
1973+
class TestPrintSkillsSummary:
1974+
def _entry(self, locations):
1975+
return mcp._resolve_skills_mcp_servers(WS, ["claude", "codex"], locations, [])[0]
1976+
1977+
def test_reports_scoped_connection(self, capsys):
1978+
mcp._print_skills_summary(self._entry(["main.default"]))
1979+
assert _unwrap(capsys.readouterr().out) == (
1980+
"✔ Skills MCP registered "
1981+
"Server: databricks-skill-registry "
1982+
f"URL: {WS}/ai-gateway/skills/?schema=main.default "
1983+
"Configured: Claude Code, Codex "
1984+
"Tools: UC skill utility tools + skills tools in schema main.default "
1985+
"• Run `ucode <agent>` to use the skills MCP. For existing sessions, "
1986+
"restart the agent for the skills to take effect."
1987+
)
1988+
1989+
def test_reports_schemaless_connection(self, capsys):
1990+
mcp._print_skills_summary(self._entry([]))
1991+
assert _unwrap(capsys.readouterr().out) == (
1992+
"✔ Skills MCP registered "
1993+
"Server: databricks-skill-registry "
1994+
f"URL: {WS}/ai-gateway/skills/ "
1995+
"Configured: Claude Code, Codex "
1996+
"Tools: UC skill utility tools "
1997+
"• Run `ucode <agent>` to use the skills MCP. For existing sessions, "
1998+
"restart the agent for the skills to take effect."
1999+
)
2000+
2001+
19532002
class TestRevertMcpConfigs:
19542003
def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkeypatch):
19552004
removed: list[tuple[str, str]] = []

tests/test_skills_download.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -337,13 +337,17 @@ def test_bundle_failure_skips_that_skill_only(self, tmp_path, monkeypatch):
337337
assert (tmp_path / ".claude/skills/good/SKILL.md").read_bytes() == b"ok"
338338
assert not (tmp_path / ".claude/skills/bad").exists()
339339

340-
def test_prints_downloaded_count_summary(self, tmp_path, monkeypatch, capsys):
340+
def test_prints_downloaded_count_and_roots_summary(self, tmp_path, monkeypatch, capsys):
341341
monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["a", "b", "c"], None))
342342
monkeypatch.setattr(sd, "fetch_skill_bundle", lambda *a, **k: ({"SKILL.md": b"x"}, None))
343343

344344
sd.download_skills(WS, "token", ["main.default"], str(tmp_path))
345345

346-
assert "Downloaded 3/3 skill(s) from `main.default`" in capsys.readouterr().out
346+
# Rich wraps long paths across lines; strip all whitespace from both sides to compare.
347+
roots = sd.skill_dir_roots(str(tmp_path))
348+
expected = f"Downloaded 3/3 skill(s) from `main.default` in {roots[0]} and {roots[1]}."
349+
printed = "".join(capsys.readouterr().out.split())
350+
assert "".join(expected.split()) in printed
347351

348352
def test_summary_counts_only_written_skills(self, tmp_path, monkeypatch, capsys):
349353
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)
357361

358362
sd.download_skills(WS, "token", ["main.default"], str(tmp_path))
359363

360-
assert "Downloaded 1/2 skill(s) from `main.default`" in capsys.readouterr().out
364+
assert "Downloaded 1/2 skill(s) from `main.default` in" in capsys.readouterr().out
361365

362366

363367
class TestConfigureSkillsDownloadCommand:
@@ -394,3 +398,4 @@ def test_none_path_threads_through(self, monkeypatch):
394398
assert sd.configure_skills_download_command(["a.b"], path=None) == 0
395399

396400
assert calls["download"] == (WS, "token", ["a.b"], None)
401+
assert calls["register"] == (WS, "profile", ["claude"])

0 commit comments

Comments
 (0)