diff --git a/src/ucode/cli.py b/src/ucode/cli.py index a0ef7c4..36afde4 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -53,7 +53,9 @@ ) from ucode.mcp import ( MCP_CLIENTS, + SKILLS_MCP_KIND, configure_mcp_command, + configure_skills_mcp_command, purge_cross_workspace_mcp_residue, revert_mcp_configs, ) @@ -142,6 +144,27 @@ def _parse_agents_option(agents: str) -> list[str]: return tools +def _parse_skill_locations(location: str) -> list[str]: + """Parse a comma-separated `--location` into `.` refs, + dropping duplicates while preserving order.""" + locations: list[str] = [] + for raw in location.split(","): + raw = raw.strip() + if not raw: + continue + parts = raw.split(".") + if len(parts) != 2 or not all(part.strip() for part in parts): + 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 + + def _parse_workspaces_option(workspaces: str) -> list[tuple[str, str | None]]: """Parse `--workspaces` into [(url, profile_name | None), ...]. @@ -697,7 +720,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 +732,23 @@ def status() -> int: print_kv("Config file", str(config_path) if config_path.exists() else "missing") console.print() + print_heading("Skills") + skill_mcp_entry = next((s for s in mcp_servers if s.get("kind") == SKILLS_MCP_KIND), None) + if not skill_mcp_entry: + print_kv("Skills", "not configured") + else: + locations = skill_mcp_entry.get("skill_locations") or [] + print_kv( + "Skill MCP Locations", + ", ".join(locations) if locations else "none — utility tools only", + ) + configured_agents = [ + str(MCP_CLIENTS[client]["display"]) + for client in (skill_mcp_entry.get("clients") or []) + if client in MCP_CLIENTS + ] + print_kv("Configured", ", ".join(configured_agents) if configured_agents else "none") + print_heading("Tracing") tracing = state.get("tracing") or {} if tracing.get("enabled"): @@ -1268,6 +1310,36 @@ 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."), + ], + mcp: Annotated[ + bool, + typer.Option( + "--mcp", help="Manage the skills MCP connection (required until download mode lands)." + ), + ] = False, +) -> None: + """Configure Databricks Skills for your coding tools. + + ``--location`` sets the skills MCP connection's scope to exactly the listed + schemas, replacing any previous set. + """ + try: + if not mcp: + raise RuntimeError("Download mode is not available yet; pass --mcp for now.") + configure_skills_mcp_command(_parse_skill_locations(location)) + 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..a337c02 100644 --- a/src/ucode/databricks.py +++ b/src/ucode/databricks.py @@ -1329,6 +1329,19 @@ 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 one ``?schema=`` scope per location. The trailing slash + is required by the Envoy prefix even with no query params. + + [] -> ``.../ai-gateway/skills/`` + ["main.default", "ml.a"] -> ``.../ai-gateway/skills/?schema=main.default&schema=ml.a`` + """ + 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..9ab931f 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,8 @@ "list_command": "copilot mcp list", }, } +SKILLS_MCP_KIND = "skills" +SKILLS_MCP_SERVER_NAME = "databricks-skill-registry" EXTERNAL_MCP_SELECTION_PREFIX = "external:" SQL_MCP_VALUE = "managed:sql" GENIE_SPACE_SELECTION_PREFIX = "genie-space:" @@ -96,14 +99,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 +1043,9 @@ 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 (Claude-only) keeps the skills registry's utility tools + # discoverable without an explicit mention; other clients ignore it. + 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 @@ -1103,6 +1111,10 @@ def purge_cross_workspace_mcp_residue(state: dict, workspace: str) -> None: ) +def _skills_entries(servers: list[dict]) -> list[dict]: + return [s for s in servers if s.get("kind") == SKILLS_MCP_KIND] + + def _resolve_location_mcp_servers( workspace: str, profile: str | None, @@ -1113,9 +1125,10 @@ def _resolve_location_mcp_servers( ) -> list[dict]: """Build the desired MCP server list for ``--location .``. - Strict replacement: the returned list is exactly the mcp-services - discovered at ``location``. Any previously-registered MCP entries outside - that location are removed by ``apply_mcp_server_changes``. Raises ``RuntimeError`` for an invalid + Strict replacement for mcp-services: the returned list is exactly the ones + discovered at ``location`` (any previously-registered mcp-service outside it + is removed by ``apply_mcp_server_changes``), plus any existing skills + connection, preserved untouched. Raises ``RuntimeError`` for an invalid location (HTTP 404 from the listing API) or any other listing failure. When ``services`` is given, the discovered set is narrowed to exactly that @@ -1174,28 +1187,15 @@ def _resolve_location_mcp_servers( working_servers.append(original.copy()) else: working_servers.append(candidate) - return working_servers + return [*working_servers, *_skills_entries(original_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 _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 +1223,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 +1231,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: @@ -1276,12 +1299,16 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None ) original_mcp_servers: list[dict] = list(state.get("mcp_servers") or []) - original_by_name = _servers_by_name(original_mcp_servers) + # Skills connections are managed by `configure skills`, so keep them out of + # the picker and carry them through untouched. + skills_servers = _skills_entries(original_mcp_servers) + picker_servers = [s for s in original_mcp_servers if s.get("kind") != SKILLS_MCP_KIND] + original_by_name = _servers_by_name(picker_servers) selections = prompt_for_mcp_server_choices( available_external_mcp_names, available_genie_mcp_servers, available_app_mcp_servers, - original_mcp_servers, + picker_servers, available_mcp_service_names, available_vector_search_servers, available_uc_functions_servers, @@ -1289,7 +1316,7 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None if selections is None: return 0 - working_mcp_servers: list[dict] = [] + working_mcp_servers: list[dict] = list(skills_servers) working_names: set[str] = set() add_selections: list[str] = [] for selection in selections: @@ -1335,3 +1362,68 @@ 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 _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 _update_skills_mcp( + 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]) -> int: + """Set the skills MCP connection's ``skill_locations`` to exactly ``locations``, + replacing any previous set.""" + state = load_state() + workspace, _profile, clients = _setup_mcp_clients(state, "Skills MCP") + _update_skills_mcp(state, workspace, clients, locations) + return 0 diff --git a/tests/test_cli.py b/tests/test_cli.py index 76a2ec5..d734111 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -342,6 +342,125 @@ 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_mcp_flag_dispatches_location_set(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"]) + + 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"]) + + def test_without_mcp_is_not_implemented_exit_1(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke(app, ["configure", "skills", "--location", "a.b"]) + assert result.exit_code == 1 + mock_mcp.assert_not_called() + + def test_three_part_location_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_malformed_location_exit_1_names_location(self): + with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + result = runner.invoke(app, ["configure", "skills", "--location", "justone", "--mcp"]) + assert result.exit_code == 1 + 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 + + +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_renders_locations_and_configured_agents(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 "Skill MCP Locations: main.default, ml.prod" in out + assert "Configured: Claude Code, Codex" in out + + def test_renders_placeholder_when_no_locations(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 "Skill MCP Locations: 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 is managed in the Skills section, never listed 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 "Skill MCP Locations: main.default" 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..a520afa 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): @@ -817,6 +825,60 @@ def test_hints_when_no_selections_and_no_existing_servers(self, monkeypatch, cap assert "space to toggle" in output assert saved_states == [] + def test_preserves_skills_connection_and_hides_it_from_picker(self, monkeypatch): + """The picker manages mcp-services only; a skills connection is kept out + of the choices and never removed. Selecting an mcp-service saves both.""" + saved_states: list[dict] = [] + picker_servers: list[list[dict]] = [] + removed: list[tuple[str, str]] = [] + skills_entry = { + "name": mcp.SKILLS_MCP_SERVER_NAME, + "kind": mcp.SKILLS_MCP_KIND, + "skill_locations": ["main.default"], + "url": f"{WS}/ai-gateway/skills/?schema=main.default", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + } + + monkeypatch.setattr( + mcp, "load_state", lambda: {**CLAUDE_STATE, "mcp_servers": [skills_entry]} + ) + monkeypatch.setattr(mcp.shutil, "which", lambda binary: f"/usr/bin/{binary}") + monkeypatch.setattr(mcp, "ensure_databricks_auth", lambda workspace, profile=None: None) + monkeypatch.setattr(mcp, "available_mcp_clients", lambda: ["claude"]) + monkeypatch.setattr( + mcp, "discover_external_mcp_connection_names", lambda workspace, profile=None: [] + ) + monkeypatch.setattr(mcp, "discover_genie_mcp_servers", lambda workspace, profile=None: []) + monkeypatch.setattr(mcp, "discover_app_mcp_servers", lambda workspace, profile=None: []) + monkeypatch.setattr(mcp, "discover_mcp_service_names", lambda workspace, profile=None: []) + monkeypatch.setattr( + mcp, "discover_vector_search_mcp_servers", lambda workspace, profile=None: [] + ) + monkeypatch.setattr( + mcp, "discover_uc_functions_mcp_servers", lambda workspace, profile=None: [] + ) + monkeypatch.setattr( + mcp, + "prompt_for_mcp_server_choices", + lambda ext, genie, app, servers, *a, **kw: ( + picker_servers.append(servers) or [f"{mcp.MCP_ADD_PREFIX}{mcp.SQL_MCP_VALUE}"] + ), + ) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a: []) + monkeypatch.setattr( + mcp, + "remove_client_mcp_server", + lambda client, name: removed.append((client, name)) or [], + ) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + assert mcp.configure_mcp_command() == 0 + + assert picker_servers == [[]] + assert removed == [] + assert skills_entry in saved_states[-1]["mcp_servers"] + def test_drops_stale_foreign_workspace_mcp_entries(self, monkeypatch, capsys): saved_states: list[dict] = [] cleanup_calls: list[tuple[str, str]] = [] @@ -1384,6 +1446,41 @@ def test_replaces_servers_outside_location(self, monkeypatch): }, ] + def test_preserves_skills_connection(self, monkeypatch): + """A skills connection is owned by `configure skills`, so `configure mcp + --location` must leave it registered rather than treating it as a removal.""" + saved_states: list[dict] = [] + removed: list[tuple[str, str]] = [] + skills_entry = { + "name": mcp.SKILLS_MCP_SERVER_NAME, + "kind": mcp.SKILLS_MCP_KIND, + "skill_locations": ["main.default"], + "url": f"{WS}/ai-gateway/skills/?schema=main.default", + "auth": "env:OAUTH_TOKEN", + "clients": ["claude"], + } + _stub_location_base( + monkeypatch, + {**CLAUDE_STATE, "mcp_servers": [skills_entry]}, + ) + monkeypatch.setattr( + mcp, + "list_mcp_services", + lambda workspace, token, parent: (["system.ai.github"], None), + ) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a: []) + monkeypatch.setattr( + mcp, + "remove_client_mcp_server", + lambda client, name: removed.append((client, name)) or [], + ) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + assert mcp.configure_mcp_command(location="system.ai") == 0 + + assert removed == [] + assert _find_skills(saved_states[-1]["mcp_servers"]) == [skills_entry] + def test_existing_entry_gets_reconfigured_for_newly_added_clients(self, monkeypatch): """An entry registered before a second agent was configured should get registered for that agent on the next --location run.""" @@ -1642,6 +1739,143 @@ 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 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_set_on_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"]) == 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_location_replaces_prior_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"]) == 0 + + assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["X.x"] + + def test_multiple_locations_set_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(["X.x", "Y.y"]) == 0 + + assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == ["X.x", "Y.y"] + + def test_preserves_mcp_service_entries_across_set(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"]) == 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 + + class TestRevertMcpConfigs: def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkeypatch): removed: list[tuple[str, str]] = []