[skills] Inline the by-name download guard; rename schema download command

Move the `--location` rejection into each command's guard block, next to the other mutual-exclusion checks, and inline the FQN validation and dispatch, dropping _download_selected_skills_route. _is_qualified_skill_name stays shared.

Rename configure_skills_download_command to configure_location_skills_download_command so its name makes clear it downloads every skill in the selected `<catalog>.<schema>` locations, distinct from configure_selected_skills_download_command.

Co-authored-by: Isaac <no-reply@databricks.com>

Author: xsh310

src/ucode/cli.py

@@ -109,8 +109,8 @@
     skill_locations_for_client,
 )
 from ucode.skills_download import (
+    configure_location_skills_download_command,
     configure_selected_skills_download_command,
-    configure_skills_download_command,
     configure_skills_download_picker_command,
     download_managed_skills_on_launch,
 )
@@ -359,26 +359,6 @@ def _is_qualified_skill_name(name: str) -> bool:
     return len(parts) == 3 and all(part and part == part.strip() for part in parts)
 
 
-def _download_selected_skills_route(
-    requested_skills: set[str], location: str | None, path: str | None, *, flag: str
-) -> None:
-    """Download the fully-qualified `requested_skills` (cross-schema) and register.
-
-    The `--skills`/`--skill` download path: names must be fully qualified so a
-    selection can span schemas without a `--location`, so passing one is an error
-    and any bare/malformed name is rejected up front. `flag` names the option in
-    those errors. Empty selections download nothing."""
-    if location is not None:
-        raise RuntimeError(f"{flag} takes fully-qualified names; drop --location.")
-    invalid = sorted(name for name in requested_skills if not _is_qualified_skill_name(name))
-    if invalid:
-        raise RuntimeError(
-            f"{flag} entries must be fully-qualified `<catalog>.<schema>.<name>` names "
-            f"(invalid: {', '.join(invalid)})."
-        )
-    configure_selected_skills_download_command(sorted(requested_skills), path)
-
-
 def _parse_workspaces_option(workspaces: str) -> list[tuple[str, str | None]]:
     """Parse `--workspaces` into [(url, profile_name | None), ...].
 
@@ -1343,11 +1323,19 @@ def skills_add(
             raise RuntimeError("--path is not supported when using --mcp")
         if mcp and requested_skills is not None:
             raise RuntimeError("--skills is not supported when using --mcp")
+        if requested_skills is not None and location is not None:
+            raise RuntimeError("--skills takes fully-qualified names; drop --location.")
         # Downloaded skills use shared directory families, so only MCP scopes can be agent-scoped.
         if not mcp and agents is not None:
             raise RuntimeError("--agents is only supported when using --mcp")
         if requested_skills is not None:
-            _download_selected_skills_route(requested_skills, location, path, flag="--skills")
+            invalid = sorted(s for s in requested_skills if not _is_qualified_skill_name(s))
+            if invalid:
+                raise RuntimeError(
+                    "--skills entries must be fully-qualified `<catalog>.<schema>.<name>` names "
+                    f"(invalid: {', '.join(invalid)})."
+                )
+            configure_selected_skills_download_command(sorted(requested_skills), path)
             return
         locations = _parse_skill_locations(location)
         if not locations:
@@ -1361,7 +1349,7 @@ def skills_add(
             )
             add_skills_command(locations, agents=scope)
         else:
-            configure_skills_download_command(locations, path=path)
+            configure_location_skills_download_command(locations, path=path)
     except (RuntimeError, ValueError) as exc:
         print_err(str(exc))
         raise typer.Exit(1) from None
@@ -3237,16 +3225,24 @@ def configure_skills(
             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 location is not None:
+            raise RuntimeError("--skill takes fully-qualified names; drop --location.")
         if selected_skills is not None:
-            _download_selected_skills_route(selected_skills, location, path, flag="--skill")
+            invalid = sorted(s for s in selected_skills if not _is_qualified_skill_name(s))
+            if invalid:
+                raise RuntimeError(
+                    "--skill entries must be fully-qualified `<catalog>.<schema>.<name>` 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_skills_download_command(locations, path=path)
+            configure_location_skills_download_command(locations, path=path)
     except (RuntimeError, ValueError) as exc:
         print_err(str(exc))
         raise typer.Exit(1) from None

src/ucode/skills_download.py

@@ -503,7 +503,7 @@ def download_managed_skills_on_launch(
     return written
 
 
-def configure_skills_download_command(locations: list[str], *, path: str | None) -> int:
+def configure_location_skills_download_command(locations: list[str], *, path: str | None) -> int:
     """Download every skill in each schema to disk and register the skills connection.
 
     Downloads to ``path`` (or the home dir when None), then registers/keeps the

tests/test_cli.py

@@ -1377,15 +1377,15 @@ def test_comma_location_yields_multiple_schemas(self):
         mock_mcp.assert_called_once_with(["a.b", "c.d"])
 
     def test_default_mode_dispatches_download_with_path(self):
-        with patch("ucode.cli.configure_skills_download_command") as mock_download:
+        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_skills_download_command") as mock_download:
+        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)
@@ -1434,7 +1434,7 @@ def test_skill_with_mcp_exit_1(self):
     def test_path_with_mcp_exit_1(self):
         with (
             patch("ucode.cli.configure_skills_mcp_command") as mock_mcp,
-            patch("ucode.cli.configure_skills_download_command") as mock_download,
+            patch("ucode.cli.configure_location_skills_download_command") as mock_download,
         ):
             result = runner.invoke(
                 app, ["configure", "skills", "--location", "a.b", "--mcp", "--path", "/tmp/skills"]
@@ -1472,7 +1472,7 @@ def test_mcp_without_location_registers_schemaless_connection(self):
     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,
+            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
@@ -1498,7 +1498,7 @@ def test_comma_location_yields_multiple_schemas(self):
         mock_add.assert_called_once_with(["a.b", "c.d"], agents=None)
 
     def test_default_mode_dispatches_download(self):
-        with patch("ucode.cli.configure_skills_download_command") as mock_download:
+        with patch("ucode.cli.configure_location_skills_download_command") as mock_download:
             result = runner.invoke(app, ["skill", "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")
@@ -1534,7 +1534,7 @@ def test_without_location_non_interactive_exit_1(self):
         with (
             patch("ucode.cli._stdin_is_interactive", return_value=False),
             patch("ucode.cli.add_skills_command") as mock_add,
-            patch("ucode.cli.configure_skills_download_command") as mock_download,
+            patch("ucode.cli.configure_location_skills_download_command") as mock_download,
             patch("ucode.cli.configure_skills_download_picker_command") as mock_picker,
         ):
             result = runner.invoke(app, ["skill", "add"])
@@ -1631,7 +1631,7 @@ def test_empty_agents_folds_to_global_scope(self):
         mock_add.assert_called_once_with(["a.b"], agents=None)
 
     def test_agents_is_rejected_for_download_mode(self):
-        with patch("ucode.cli.configure_skills_download_command") as mock_download:
+        with patch("ucode.cli.configure_location_skills_download_command") as mock_download:
             result = runner.invoke(app, ["skill", "add", "--location", "a.b", "--agents", "claude"])
 
         assert result.exit_code == 1

tests/test_skills_download.py

@@ -842,7 +842,7 @@ def test_malformed_location_is_skipped(self, tmp_path, monkeypatch):
         )
 
 
-class TestConfigureSkillsDownloadCommand:
+class TestConfigureLocationSkillsDownloadCommand:
     def _stub(self, monkeypatch):
         calls: dict[str, object] = {}
         monkeypatch.setattr(sd, "load_state", lambda: {"state": True})
@@ -865,15 +865,15 @@ def _stub(self, monkeypatch):
     def test_downloads_then_registers_connection(self, monkeypatch):
         calls = self._stub(monkeypatch)
 
-        assert sd.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0
+        assert sd.configure_location_skills_download_command(["a.b"], path="/tmp/skills") == 0
 
         assert calls["download"] == (WS, "token", ["a.b"], "/tmp/skills")
         assert calls["register"] == (WS, "profile", ["claude"])
 
     def test_none_path_threads_through(self, monkeypatch):
         calls = self._stub(monkeypatch)
 
-        assert sd.configure_skills_download_command(["a.b"], path=None) == 0
+        assert sd.configure_location_skills_download_command(["a.b"], path=None) == 0
 
         assert calls["download"] == (WS, "token", ["a.b"], None)
         assert calls["register"] == (WS, "profile", ["claude"])