diff --git a/src/ucode/cli.py b/src/ucode/cli.py index a0ef7c4..d44fa6c 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -3,7 +3,7 @@ from __future__ import annotations -from typing import Annotated +from typing import Annotated, NamedTuple import typer from rich.panel import Panel @@ -53,7 +53,10 @@ ) from ucode.mcp import ( MCP_CLIENTS, + SKILLS_MCP_KIND, configure_mcp_command, + configure_skills_command, + configure_skills_mcp_command, purge_cross_workspace_mcp_residue, revert_mcp_configs, ) @@ -142,6 +145,52 @@ def _parse_agents_option(agents: str) -> list[str]: return tools +class SkillLocation(NamedTuple): + catalog: str + schema: str + skill: str | None + + @property + def schema_ref(self) -> str: + return f"{self.catalog}.{self.schema}" + + +def _parse_skill_locations(location: str, *, allow_skill: bool) -> list[SkillLocation]: + """Parse a comma-separated `--location` into `SkillLocation`s. + + Each entry is a 2-part `.` or 3-part + `..` dotted ref with non-empty parts. When + ``allow_skill`` is False the 3-part form is rejected. Duplicates are + dropped, preserving order. + """ + locations: list[SkillLocation] = [] + seen: set[SkillLocation] = set() + for raw in location.split(","): + raw = raw.strip() + if not raw: + continue + parts = raw.split(".") + if len(parts) not in (2, 3) or not all(part.strip() for part in parts): + raise RuntimeError( + f"--location entries must be `.[.]`, got `{raw}`." + ) + if len(parts) == 3 and not allow_skill: + raise RuntimeError( + f"`.{parts[2]}` names a single skill, which is download-only and not valid " + f"with --mcp; drop it to scope the connection to `{parts[0]}.{parts[1]}`." + ) + parsed = SkillLocation(parts[0], parts[1], parts[2] if len(parts) == 3 else None) + if parsed not in seen: + seen.add(parsed) + locations.append(parsed) + if not locations: + raise RuntimeError( + "No skills provided for --location. Use `.[.]`, " + "comma-separated for multiple." + ) + return locations + + def _parse_workspaces_option(workspaces: str) -> list[tuple[str, str | None]]: """Parse `--workspaces` into [(url, profile_name | None), ...]. @@ -697,7 +746,9 @@ def status() -> int: tool_mcp_servers = [ str(server.get("name")) for server in mcp_servers - if tool in (server.get("clients") or []) and server.get("name") + if tool in (server.get("clients") or []) + and server.get("name") + and server.get("kind") != SKILLS_MCP_KIND ] print_kv("MCP list command", str(MCP_CLIENTS[tool]["list_command"])) print_kv( @@ -707,6 +758,25 @@ def status() -> int: print_kv("Config file", str(config_path) if config_path.exists() else "missing") console.print() + print_heading("Skills") + skills_entry = next((s for s in mcp_servers if s.get("kind") == SKILLS_MCP_KIND), None) + if not skills_entry: + print_kv("Skills", "not configured") + else: + locations = skills_entry.get("skill_locations") or [] + print_kv("Connection", str(skills_entry.get("name"))) + print_kv("Mode", "mcp" if locations else "download") + print_kv( + "Locations", + ", ".join(locations) if locations else "none — utility tools only", + ) + displays = [ + str(MCP_CLIENTS[client]["display"]) + for client in (skills_entry.get("clients") or []) + if client in MCP_CLIENTS + ] + print_kv("Configured", ", ".join(displays) if displays else "none") + print_heading("Tracing") tracing = state.get("tracing") or {} if tracing.get("enabled"): @@ -1268,6 +1338,65 @@ def configure_mcp( raise typer.Exit(130) from None +@configure_app.command("skills") +def configure_skills( + location: Annotated[ + str, + typer.Option( + "--location", + help="Comma-separated `.` skill scopes (a single " + "`..` is download-only). Required.", + ), + ], + mcp: Annotated[ + bool, + typer.Option("--mcp", help="Mutate the skills MCP connection instead of downloading."), + ] = False, + remove: Annotated[ + bool, typer.Option("--remove", help="(--mcp) Remove the listed schemas from the set.") + ] = False, + replace: Annotated[ + bool, + typer.Option("--replace", help="(--mcp) Replace the set with exactly the listed schemas."), + ] = False, + path: Annotated[ + str | None, typer.Option("--path", help="(download) Project root; default user scope (~/).") + ] = None, + yes: Annotated[ + bool, + typer.Option("--yes", "-y", help="(download) Overwrite on collision without prompting."), + ] = False, +) -> None: + """Configure Databricks Skills for your coding tools.""" + try: + if remove and replace: + raise RuntimeError("Use either --remove or --replace, not both.") + if (remove or replace) and not mcp: + raise RuntimeError("--remove/--replace require --mcp.") + if path is not None and mcp: + raise RuntimeError("--path is not valid with --mcp.") + if mcp: + mode = "remove" if remove else "replace" if replace else "add" + locs = _parse_skill_locations(location, allow_skill=False) + configure_skills_mcp_command([loc.schema_ref for loc in locs], mode=mode) + else: + # Download mode lands in a later PR; until then, reject every + # download-only affordance rather than silently ignoring it. + locs = _parse_skill_locations(location, allow_skill=True) + if path is not None or yes or any(loc.skill for loc in locs): + raise RuntimeError( + "Download mode is not available yet; use --mcp to configure the skills " + "connection for the given schemas." + ) + configure_skills_command([loc.schema_ref for loc in locs]) + except RuntimeError as exc: + print_err(str(exc)) + raise typer.Exit(1) from None + except KeyboardInterrupt: + print_err("Interrupted.") + raise typer.Exit(130) from None + + @configure_app.command("tracing") def configure_tracing( disable: Annotated[ diff --git a/src/ucode/databricks.py b/src/ucode/databricks.py index b38bca9..7fcc263 100644 --- a/src/ucode/databricks.py +++ b/src/ucode/databricks.py @@ -1329,6 +1329,18 @@ def build_mcp_service_url(workspace: str, full_name: str) -> str: return f"{workspace}/ai-gateway/mcp-services/{full_name}" +def build_skills_mcp_url(workspace: str, locations: list[str]) -> str: + """Skills route with repeated ``?schema=`` scopes (universe PR #2256555). + + Empty ``locations`` -> the bare route (utility tools only). The trailing + slash is required by the Envoy prefix even with no query params. + """ + base = f"{workspace}/ai-gateway/skills/" + if not locations: + return base + return base + "?" + urlencode([("schema", loc) for loc in locations]) + + # Maps the gateway routing dialect a coding tool speaks to the Model Provider # Service `provider_type`s it can be backed by. claude speaks Anthropic's API, # which both the `anthropic` and `amazon_bedrock` provider types serve (Bedrock diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 6257458..9016dab 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -29,6 +29,7 @@ from ucode.databricks import ( apply_pat_environment, build_mcp_service_url, + build_skills_mcp_url, ensure_databricks_auth, get_databricks_token, list_databricks_apps, @@ -79,6 +80,9 @@ "list_command": "copilot mcp list", }, } +SKILLS_MCP_KIND = "skills" +SKILLS_MCP_SERVER_NAME = "databricks-skill-registry" +SKILLS_MCP_SCHEMA_SOFT_CAP = 10 EXTERNAL_MCP_SELECTION_PREFIX = "external:" SQL_MCP_VALUE = "managed:sql" GENIE_SPACE_SELECTION_PREFIX = "genie-space:" @@ -96,14 +100,17 @@ ) -def build_mcp_http_entry(url: str) -> dict: - return { +def build_mcp_http_entry(url: str, *, always_load: bool = False) -> dict: + entry: dict[str, Any] = { "type": "http", "url": url, "headers": { "Authorization": f"Bearer ${{{MCP_AUTH_TOKEN_ENV_VAR}}}", }, } + if always_load: + entry["alwaysLoad"] = True + return entry def add_claude_mcp_server(name: str, entry: dict, scope: str = MCP_USER_SCOPE) -> None: @@ -1037,7 +1044,10 @@ def apply_mcp_server_changes( url = server.get("url") if not isinstance(url, str) or not url: continue - entry = build_mcp_http_entry(url) + # alwaysLoad is Claude-only and rides the per-entry apply; other clients + # get URL only. The skills registry always loads so its utility tools + # are available without an explicit MCP-server mention. + entry = build_mcp_http_entry(url, always_load=server.get("kind") == SKILLS_MCP_KIND) for client in clients: configure_client_mcp_server(client, name, url, entry) changed = True @@ -1177,25 +1187,84 @@ def _resolve_location_mcp_servers( return working_servers -def configure_mcp_command(location: str | None = None, services: set[str] | None = None) -> int: - if services is not None and location is None: - # `--services` works standalone with full names (`system.ai.github`): the - # `.` to configure is derived from them. Bare short names - # (`github`) can't be located without `--location`. - schemas = {".".join(s.split(".")[:2]) for s in services if s.count(".") >= 2} - bare = sorted(s for s in services if s.count(".") < 2) - if bare: - raise RuntimeError( - "--services short names need --location (or pass full names like " - f"`system.ai.`): {', '.join(bare)}" - ) - if len(schemas) != 1: - raise RuntimeError( - "--services without --location must all share one `.` " - f"(got: {', '.join(sorted(schemas)) or 'none'}); pass --location instead." - ) - location = next(iter(schemas)) - state = load_state() +def _merge_clients(prior: list[str] | None, new: list[str]) -> list[str]: + """Order-preserving union of a prior client list with newly-configured ones.""" + prior = list(prior or []) + return prior + [c for c in new if c not in prior] + + +def _build_skills_entry(workspace: str, locations: list[str], clients: list[str]) -> dict: + """Canonical single skills-registry entry. ``skill_locations`` is the source + of truth; the URL is always derived from it, never parsed back.""" + return { + "name": SKILLS_MCP_SERVER_NAME, + "kind": SKILLS_MCP_KIND, + "skill_locations": list(locations), + "url": build_skills_mcp_url(workspace, locations), + "auth": f"env:{MCP_AUTH_TOKEN_ENV_VAR}", + "clients": clients, + } + + +def _resolve_skills_mcp_servers( + workspace: str, + clients: list[str], + locations: list[str], + original_servers: list[dict], +) -> list[dict]: + """Rebuild the MCP server list around exactly one skills entry. + + Drops every prior ``kind=="skills"`` entry and any entry named + ``SKILLS_MCP_SERVER_NAME`` (single-connection invariant; also sweeps up a + stray old-named entry via ``apply_mcp_server_changes``), keeps everything + else, and appends one rebuilt entry whose clients merge the prior skills + entry's clients with ``clients``. + """ + prior = next((s for s in original_servers if s.get("kind") == SKILLS_MCP_KIND), None) + merged = _merge_clients((prior or {}).get("clients"), clients) + kept = [ + s + for s in original_servers + if s.get("kind") != SKILLS_MCP_KIND and _server_name(s) != SKILLS_MCP_SERVER_NAME + ] + return [*kept, _build_skills_entry(workspace, locations, merged)] + + +def _dedupe_stable(values: list[str]) -> list[str]: + seen: set[str] = set() + result: list[str] = [] + for value in values: + if value not in seen: + seen.add(value) + result.append(value) + return result + + +def resolve_skill_location_set(current: list[str], requested: list[str], *, mode: str) -> list[str]: + """Apply a set mutation to the skill-location list. ``mode`` is one of + ``add`` (union), ``remove`` (difference), ``replace`` (deduped requested). + Order is stable throughout; backend assigns tool indices by URL position.""" + if mode == "remove": + return [c for c in current if c not in requested] + if mode == "replace": + return _dedupe_stable(requested) + return current + [r for r in requested if r not in current] + + +def _current_skill_locations(state: dict) -> list[str]: + """Read ``skill_locations`` off the single ``kind:"skills"`` entry, else ``[]``.""" + for server in state.get("mcp_servers") or []: + if server.get("kind") == SKILLS_MCP_KIND: + return list(server.get("skill_locations") or []) + return [] + + +def _setup_mcp_clients(state: dict, section: str) -> tuple[str, str | None, list[str]]: + """Validate the workspace, resolve configured MCP clients, and prepare auth. + + Returns ``(workspace, profile, clients)`` and prints the section header, the + "Configuring for" note, and a warning per configured-but-uninstalled client. + """ workspace = state.get("workspace") if not workspace: raise RuntimeError("Workspace is not configured. Run `ucode configure` first.") @@ -1223,7 +1292,7 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None apply_pat_environment(state) ensure_databricks_auth(workspace, profile) - print_section("MCP Servers") + print_section(section) client_names = ", ".join(str(MCP_CLIENTS[client]["display"]) for client in clients) print_note(f"Configuring for: {client_names}") for client in missing_clients: @@ -1231,6 +1300,29 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None f"{MCP_CLIENTS[client]['display']} is configured in ucode but not installed; " "skipping MCP config." ) + return workspace, profile, clients + + +def configure_mcp_command(location: str | None = None, services: set[str] | None = None) -> int: + if services is not None and location is None: + # `--services` works standalone with full names (`system.ai.github`): the + # `.` to configure is derived from them. Bare short names + # (`github`) can't be located without `--location`. + schemas = {".".join(s.split(".")[:2]) for s in services if s.count(".") >= 2} + bare = sorted(s for s in services if s.count(".") < 2) + if bare: + raise RuntimeError( + "--services short names need --location (or pass full names like " + f"`system.ai.`): {', '.join(bare)}" + ) + if len(schemas) != 1: + raise RuntimeError( + "--services without --location must all share one `.` " + f"(got: {', '.join(sorted(schemas)) or 'none'}); pass --location instead." + ) + location = next(iter(schemas)) + state = load_state() + workspace, profile, clients = _setup_mcp_clients(state, "MCP Servers") original_mcp_servers_for_location: list[dict] = list(state.get("mcp_servers") or []) if location is not None: @@ -1335,3 +1427,40 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None # User submitted the picker without toggling anything --> make it clear nothing was selected print_note("No MCP servers selected. Press space to toggle an item, then enter to save.") return 0 + + +def _apply_skills_connection( + state: dict, workspace: str, clients: list[str], locations: list[str] +) -> None: + """Rebuild the single skills connection for ``locations`` and persist it.""" + original = list(state.get("mcp_servers") or []) + working = _resolve_skills_mcp_servers(workspace, clients, locations, original) + changed = apply_mcp_server_changes(original, working, clients) + if changed or original != working: + state["mcp_servers"] = working + save_state(state) + print_success("Saved") + + +def configure_skills_mcp_command(locations: list[str], *, mode: str) -> int: + """Add/remove/replace the skills MCP connection's ``skill_locations`` set.""" + state = load_state() + workspace, _profile, clients = _setup_mcp_clients(state, "Skills MCP") + new_locations = resolve_skill_location_set( + _current_skill_locations(state), locations, mode=mode + ) + if len(new_locations) > SKILLS_MCP_SCHEMA_SOFT_CAP: + print_warning( + f"{len(new_locations)} schemas exceeds the recommended limit of " + f"{SKILLS_MCP_SCHEMA_SOFT_CAP}; registering anyway." + ) + _apply_skills_connection(state, workspace, clients, new_locations) + return 0 + + +def configure_skills_command(locations: list[str]) -> int: + """Bare-default entry point: register the skills connection for ``locations``.""" + state = load_state() + workspace, _profile, clients = _setup_mcp_clients(state, "Skills") + _apply_skills_connection(state, workspace, clients, locations) + return 0 diff --git a/tests/test_cli.py b/tests/test_cli.py index 76a2ec5..8599e23 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -342,6 +342,191 @@ def test_status_treats_available_tools_as_configured_agents(self): assert "https://example.databricks.com/ai-gateway/gemini" not in result.output +class TestConfigureSkillsCommand: + def test_default_calls_configure_skills_command(self): + with ( + patch("ucode.cli.configure_skills_command") as mock_default, + patch("ucode.cli.configure_skills_mcp_command") as mock_mcp, + ): + result = runner.invoke(app, ["configure", "skills", "--location", "a.b"]) + assert result.exit_code == 0, result.output + mock_default.assert_called_once_with(["a.b"]) + mock_mcp.assert_not_called() + + def test_mcp_flag_dispatches_add_mode(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"], mode="add") + + def test_mcp_remove_dispatches_remove_mode(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke( + app, ["configure", "skills", "--location", "a.b", "--mcp", "--remove"] + ) + assert result.exit_code == 0, result.output + mock_mcp.assert_called_once_with(["a.b"], mode="remove") + + def test_mcp_replace_dispatches_replace_mode(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke( + app, ["configure", "skills", "--location", "a.b", "--mcp", "--replace"] + ) + assert result.exit_code == 0, result.output + mock_mcp.assert_called_once_with(["a.b"], mode="replace") + + 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"], mode="add") + + def test_remove_and_replace_together_exit_1(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke( + app, + ["configure", "skills", "--location", "a.b", "--mcp", "--remove", "--replace"], + ) + assert result.exit_code == 1 + mock_mcp.assert_not_called() + + def test_remove_without_mcp_exit_1(self): + with patch("ucode.cli.configure_skills_command") as mock_default: + result = runner.invoke(app, ["configure", "skills", "--location", "a.b", "--remove"]) + assert result.exit_code == 1 + mock_default.assert_not_called() + + def test_path_with_mcp_exit_1(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke( + app, ["configure", "skills", "--location", "a.b", "--mcp", "--path", "/tmp/p"] + ) + assert result.exit_code == 1 + mock_mcp.assert_not_called() + + def test_three_part_with_mcp_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_three_part_without_mcp_is_download_only_exit_1(self): + with patch("ucode.cli.configure_skills_command") as mock_default: + result = runner.invoke(app, ["configure", "skills", "--location", "a.b.c"]) + assert result.exit_code == 1 + mock_default.assert_not_called() + + def test_path_without_mcp_is_download_only_exit_1(self): + with patch("ucode.cli.configure_skills_command") as mock_default: + result = runner.invoke( + app, ["configure", "skills", "--location", "a.b", "--path", "/tmp/p"] + ) + assert result.exit_code == 1 + mock_default.assert_not_called() + + def test_yes_without_mcp_is_download_only_exit_1(self): + with patch("ucode.cli.configure_skills_command") as mock_default: + result = runner.invoke(app, ["configure", "skills", "--location", "a.b", "--yes"]) + assert result.exit_code == 1 + mock_default.assert_not_called() + + def test_malformed_location_exit_1_names_location(self): + with patch("ucode.cli.configure_skills_command") as mock_default: + result = runner.invoke(app, ["configure", "skills", "--location", "justone"]) + assert result.exit_code == 1 + assert "--location" in _strip_ansi(result.output) + mock_default.assert_not_called() + + def test_missing_location_is_typer_usage_error(self): + result = runner.invoke(app, ["configure", "skills"]) + assert result.exit_code == 2 + + +class TestStatusSkillsSection: + def _run(self, state): + with patch("ucode.cli.load_state", return_value=state): + return runner.invoke(app, ["status"]) + + def test_not_configured_when_no_skills_entry(self): + result = self._run(MINIMAL_STATE) + assert result.exit_code == 0, result.output + out = _strip_ansi(result.output) + assert "Skills" in out + assert "not configured" in out + + def test_mcp_mode_rendering(self): + state = { + **MINIMAL_STATE, + "mcp_servers": [ + { + "name": "databricks-skill-registry", + "kind": "skills", + "skill_locations": ["main.default", "ml.prod"], + "url": "https://example.databricks.com/ai-gateway/skills/?schema=main.default&schema=ml.prod", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude", "codex"], + } + ], + } + result = self._run(state) + assert result.exit_code == 0, result.output + out = _strip_ansi(result.output) + assert "Connection: databricks-skill-registry" in out + assert "Mode: mcp" in out + assert "Locations: main.default, ml.prod" in out + assert "Configured: Claude Code, Codex" in out + + def test_download_mode_rendering(self): + state = { + **MINIMAL_STATE, + "mcp_servers": [ + { + "name": "databricks-skill-registry", + "kind": "skills", + "skill_locations": [], + "url": "https://example.databricks.com/ai-gateway/skills/", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + } + ], + } + result = self._run(state) + assert result.exit_code == 0, result.output + out = _strip_ansi(result.output) + assert "Mode: download" in out + assert "none — utility tools only" in out + + def test_skills_entry_absent_from_per_client_mcp_lines(self): + state = { + **MINIMAL_STATE, + "mcp_servers": [ + { + "name": "github-mcp", + "url": "https://example.databricks.com/api/2.0/mcp/external/github-mcp", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + }, + { + "name": "databricks-skill-registry", + "kind": "skills", + "skill_locations": ["main.default"], + "url": "https://example.databricks.com/ai-gateway/skills/?schema=main.default", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + }, + ], + } + result = self._run(state) + assert result.exit_code == 0, result.output + out = _strip_ansi(result.output) + # The skills registry appears only in the Skills section, never on a + # per-client "MCP servers:" line. + for line in out.splitlines(): + if "MCP servers:" in line: + assert "databricks-skill-registry" not in line + assert "Connection: databricks-skill-registry" in out + + class TestRevert: def test_reverts_mcp_configs_before_clearing_state(self): state = { diff --git a/tests/test_databricks.py b/tests/test_databricks.py index 40af048..0d4ebf5 100644 --- a/tests/test_databricks.py +++ b/tests/test_databricks.py @@ -21,6 +21,7 @@ build_databricks_cli_env, build_opencode_base_urls, build_shared_base_urls, + build_skills_mcp_url, build_tool_base_url, ensure_databricks_cli_version, ensure_pat_bearer, @@ -118,6 +119,21 @@ def test_codex_url_format(self): assert urls["codex"] == f"{WS}/ai-gateway/codex/v1" +class TestBuildSkillsMcpUrl: + def test_empty_locations_returns_bare_route(self): + assert build_skills_mcp_url(WS, []) == f"{WS}/ai-gateway/skills/" + + def test_single_location_appends_schema_query(self): + assert build_skills_mcp_url(WS, ["main.default"]) == ( + f"{WS}/ai-gateway/skills/?schema=main.default" + ) + + def test_multiple_locations_preserve_order(self): + assert build_skills_mcp_url(WS, ["a.b", "c.d"]) == ( + f"{WS}/ai-gateway/skills/?schema=a.b&schema=c.d" + ) + + class TestDiscoverClaudeModels: def test_selects_opus_4_8_when_advertised(self, monkeypatch): payload = { diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 81333c8..5a69624 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -25,6 +25,14 @@ def test_uses_oauth_token_header_reference(self): assert "oauth" not in entry assert "headersHelper" not in entry + def test_omits_always_load_by_default(self): + entry = mcp.build_mcp_http_entry(f"{WS}/ai-gateway/skills/") + assert "alwaysLoad" not in entry + + def test_sets_always_load_when_requested(self): + entry = mcp.build_mcp_http_entry(f"{WS}/ai-gateway/skills/", always_load=True) + assert entry["alwaysLoad"] is True + class TestAddClaudeMcpServer: def test_adds_user_scoped_json(self, monkeypatch): @@ -1642,6 +1650,218 @@ def test_full_names_spanning_multiple_schemas_without_location_raises(self): ) +def _find_skills(servers): + return [s for s in servers if s.get("kind") == mcp.SKILLS_MCP_KIND] + + +class TestResolveSkillLocationSet: + def test_add_is_order_preserving_union(self): + assert mcp.resolve_skill_location_set(["a.b"], ["c.d", "a.b"], mode="add") == [ + "a.b", + "c.d", + ] + + def test_add_to_empty(self): + assert mcp.resolve_skill_location_set([], ["a.b"], mode="add") == ["a.b"] + + def test_remove_is_difference(self): + assert mcp.resolve_skill_location_set(["a.b", "c.d"], ["c.d"], mode="remove") == ["a.b"] + + def test_replace_dedupes_preserving_order(self): + assert mcp.resolve_skill_location_set(["a.b"], ["x.y", "x.y", "z.w"], mode="replace") == [ + "x.y", + "z.w", + ] + + def test_replace_with_empty_clears(self): + assert mcp.resolve_skill_location_set(["a.b"], [], mode="replace") == [] + + +class TestResolveSkillsMcpServers: + def test_builds_single_canonical_entry(self): + servers = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["main.default"], []) + assert _find_skills(servers) == servers + entry = servers[0] + assert entry["name"] == mcp.SKILLS_MCP_SERVER_NAME + assert entry["kind"] == mcp.SKILLS_MCP_KIND + assert entry["skill_locations"] == ["main.default"] + assert entry["url"] == f"{WS}/ai-gateway/skills/?schema=main.default" + assert entry["auth"] == "env:OAUTH_TOKEN" + assert entry["clients"] == ["claude"] + + def test_keeps_other_entries_and_rebuilds_to_one_skills_entry(self): + service_entry = { + "name": "system-ai-github", + "url": f"{WS}/ai-gateway/mcp-services/system.ai.github", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + } + stale_skills = { + "name": mcp.SKILLS_MCP_SERVER_NAME, + "kind": mcp.SKILLS_MCP_KIND, + "skill_locations": ["old.one"], + "url": f"{WS}/ai-gateway/skills/?schema=old.one", + "auth": "env:OAUTH_TOKEN", + "clients": ["codex"], + } + servers = mcp._resolve_skills_mcp_servers( + WS, ["claude"], ["a.b"], [service_entry, stale_skills] + ) + assert service_entry in servers + skills = _find_skills(servers) + assert len(skills) == 1 + # Prior skills clients merge with the newly-configured ones (order-stable). + assert skills[0]["clients"] == ["codex", "claude"] + + def test_drops_entry_matching_skills_server_name_even_without_kind(self): + old_named = { + "name": mcp.SKILLS_MCP_SERVER_NAME, + "url": f"{WS}/ai-gateway/skills/", + "clients": ["claude"], + } + servers = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["a.b"], [old_named]) + assert len(servers) == 1 + assert servers[0]["kind"] == mcp.SKILLS_MCP_KIND + + def test_url_derives_from_locations_not_stale_url(self): + stale = { + "name": mcp.SKILLS_MCP_SERVER_NAME, + "kind": mcp.SKILLS_MCP_KIND, + "skill_locations": ["old.one"], + "url": f"{WS}/ai-gateway/skills/?schema=stale.value", + "clients": ["claude"], + } + servers = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["new.two"], [stale]) + assert servers[0]["url"] == f"{WS}/ai-gateway/skills/?schema=new.two" + + def test_empty_locations_yields_bare_route(self): + servers = mcp._resolve_skills_mcp_servers(WS, ["claude"], [], []) + assert servers[0]["skill_locations"] == [] + assert servers[0]["url"] == f"{WS}/ai-gateway/skills/" + + +def _skills_state(mcp_servers=None): + state = {"workspace": WS, "available_tools": ["claude"]} + if mcp_servers is not None: + state["mcp_servers"] = mcp_servers + return state + + +class TestConfigureSkillsMcpCommand: + def test_add_to_empty_registers_connection(self, monkeypatch): + saved_states: list[dict] = [] + configured: list[tuple[str, str, str, dict]] = [] + _stub_location_base(monkeypatch, _skills_state()) + monkeypatch.setattr( + mcp, + "configure_client_mcp_server", + lambda client, name, url, entry: configured.append((client, name, url, entry)) or [], + ) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + assert mcp.configure_skills_mcp_command(["a.b"], mode="add") == 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 rides the per-entry apply (Claude-only). + assert configured[0][3]["alwaysLoad"] is True + + def test_add_second_appends_in_order(self, monkeypatch): + saved_states: list[dict] = [] + prior = mcp._resolve_skills_mcp_servers(WS, ["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(["B.b"], mode="add") == 0 + + assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["A.a", "B.b"] + + def test_remove_from_set(self, monkeypatch): + saved_states: list[dict] = [] + prior = mcp._resolve_skills_mcp_servers(WS, ["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(["B.b"], mode="remove") == 0 + + assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["A.a"] + + def test_replace_set(self, monkeypatch): + saved_states: list[dict] = [] + prior = mcp._resolve_skills_mcp_servers(WS, ["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"], mode="replace") == 0 + + assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["X.x"] + + def test_remove_last_keeps_entry_with_bare_url(self, monkeypatch): + saved_states: list[dict] = [] + prior = mcp._resolve_skills_mcp_servers(WS, ["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(["A.a"], mode="remove") == 0 + + skills = _find_skills(saved_states[-1]["mcp_servers"]) + assert len(skills) == 1 + assert skills[0]["skill_locations"] == [] + assert skills[0]["url"] == f"{WS}/ai-gateway/skills/" + + def test_preserves_mcp_service_entries_across_mutations(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"], ["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"], mode="add") == 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 + + def test_soft_warns_over_schema_cap(self, monkeypatch, capsys): + saved_states: list[dict] = [] + _stub_location_base(monkeypatch, _skills_state()) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + many = [f"c.s{i}" for i in range(mcp.SKILLS_MCP_SCHEMA_SOFT_CAP + 1)] + assert mcp.configure_skills_mcp_command(many, mode="replace") == 0 + + assert len(_find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"]) == len(many) + out = capsys.readouterr().out + assert "recommended limit" in out + + +class TestConfigureSkillsCommand: + def test_bare_default_registers_connection(self, monkeypatch): + saved_states: list[dict] = [] + _stub_location_base(monkeypatch, _skills_state()) + 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_command(["a.b"]) == 0 + + skills = _find_skills(saved_states[-1]["mcp_servers"]) + assert len(skills) == 1 + assert skills[0]["skill_locations"] == ["a.b"] + + class TestRevertMcpConfigs: def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkeypatch): removed: list[tuple[str, str]] = []