diff --git a/AGENTS.md b/AGENTS.md index 6a2872b1..f9353018 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,7 +20,7 @@ Tests live in `tests/`. - Use Python 3.12+. - Keep changes scoped to the requested behavior. -- Follow the existing module boundaries: CLI orchestration in `cli.py`, agent-specific behavior in `agents/.py`, shared agent dispatch in `agents/__init__.py`, Databricks calls in `databricks.py`, skill download (UC fetch client + on-disk writer + download orchestration) in `skills_download.py`, MCP-connection state glue in `mcp.py`, and presentation helpers in `ui.py`. Skill download persists no disk state — it writes files to `--path` (or the home dir) and registers only the schema-less skills MCP connection. `ucode configure skills` with no `--location` (or `--mcp` with no `--location`) registers that schema-less connection without downloading anything. +- Follow the existing module boundaries: CLI orchestration in `cli.py`, agent-specific behavior in `agents/.py`, shared agent dispatch in `agents/__init__.py`, Databricks calls in `databricks.py`, skill download (UC fetch client + on-disk writer + download orchestration) in `skills_download.py`, MCP-connection state glue in `mcp.py`, and presentation helpers in `ui.py`. Skill download persists no disk state — it writes files to `--path` (or the home dir) and registers only the schema-less skills MCP connection. `ucode skills` with no `--location` (or `--mcp` with no `--location`) registers that schema-less connection without downloading anything. - Prefer existing helpers for config file writes, state persistence, UI messages, and Databricks authentication. - Add or update focused tests for behavior changes. - Do not modify generated or lock files unless the dependency graph intentionally changes. diff --git a/README.md b/README.md index 661ab410..63ef68d1 100644 --- a/README.md +++ b/README.md @@ -103,10 +103,10 @@ ucode configure --profiles DEFAULT --agents claude,codex --use-pat --skip-valida ### MCP servers (optional) ```bash -ucode configure mcp +ucode mcp ``` -Add Databricks MCP servers to installed MCP-capable tools: Codex, Claude Code, Gemini CLI, OpenCode, GitHub Copilot CLI, and Cursor Agent. +Add Databricks MCP servers to installed MCP-capable tools: Codex, Claude Code, Gemini CLI, OpenCode, GitHub Copilot CLI, and Cursor Agent. MCP servers are personal, per-developer configuration — they are not part of a workspace's managed config. Options are shown in this order: - Discovered external MCP connections @@ -125,21 +125,15 @@ The coding tool starts and stops the proxy as a child process; there's nothing e **Cursor** is MCP-only: `cursor-agent` runs models on your own Cursor account, so `ucode` configures no models for it — it only registers Databricks MCP servers in `~/.cursor/mcp.json` (via the same proxy). Include it with `ucode configure --agents cursor` or pick it in -`ucode configure mcp`, then launch with `ucode cursor`. +`ucode mcp`, then launch with `ucode cursor`. -To set up an agent and its MCP server(s) in one command, pass `--mcp` with fully-qualified -service name(s) to `ucode configure`: - -```bash -ucode configure --agents claude --mcp system.ai.slack -``` - -`--mcp` also works without `--agents` for MCP-only clients (it configures just the workspace, -then registers the servers); pass a comma-separated list to register several at once. +MCP configuration lives entirely under `ucode mcp` — it is separate from `ucode configure`, which +sets up workspaces and agent models. To register a specific service non-interactively, use +`ucode mcp --services system.ai.slack` (or `ucode mcp add --services …` to keep existing servers). #### Add servers without replacing existing ones -`ucode configure mcp` **replaces** the registered MCP servers with your selection — anything +`ucode mcp` **replaces** the registered MCP servers with your selection — anything outside a `--location`/`--services` scope (or left unchecked in the picker) is removed. To **add** servers while leaving everything already configured in place, use `ucode mcp add`: @@ -147,14 +141,14 @@ outside a `--location`/`--services` scope (or left unchecked in the picker) is r # Register a whole schema's services, keeping any servers already configured. ucode mcp add --location system.ai -# Register just a subset (same name rules as `configure mcp --services`). +# Register just a subset (same name rules as `ucode mcp --services`). ucode mcp add --services system.ai.slack,system.ai.github # No arguments launches the same interactive picker, but never removes servers. ucode mcp add ``` -`ucode mcp add` takes the same `--location` and `--services` options as `ucode configure mcp`; +`ucode mcp add` takes the same `--location` and `--services` options as bare `ucode mcp`; the only difference is that it never removes servers outside the selection. In the interactive picker, servers you already have configured are shown as `(already configured)` and can't be toggled off — you only pick new ones to add. @@ -189,20 +183,21 @@ on — and removes the ones you select from those tools. It needs no Databricks ### Skills (optional) -Configure Unity Catalog Skills for your coding tools with `ucode configure skills`: +Configure Unity Catalog Skills for your coding tools with `ucode skills`. Skills are personal, +per-developer configuration — they are not part of a workspace's managed config. ```bash # Utility tools only: register the schema-less skills MCP connection, no download. -ucode configure skills +ucode skills # Download mode: fetch every skill in the schema to disk (and register the connection). -ucode configure skills --location main.default --path /abs/project/dir +ucode skills --location main.default --path /abs/project/dir # Download a named subset of the schema's skills instead of all of them. -ucode configure skills --location main.default --skill my-skill +ucode skills --location main.default --skill my-skill # MCP mode: expose the schema's skills as MCP tools instead of downloading. -ucode configure skills --location main.default,ml.prod --mcp +ucode skills --location main.default,ml.prod --mcp ``` - **Bare command** (no `--location`) registers the schema-less skills MCP connection — the @@ -226,17 +221,19 @@ you to run `ucode ` (existing agent sessions need a restart before the MC Author the coding config your developers pick up automatically, instead of asking each of them to run `ucode configure` by hand. Restricted to workspace admins. `ucode setup help` prints the whole -sequence; the short version is one command for the agents and models, then a command per optional -section, then publish: +sequence; the short version is one command for the agents and models, then the spend-tier command, +then publish: ```bash ucode setup # agents and models (start here) -ucode setup mcps # managed MCP servers -ucode setup skills # managed skills ucode setup spend-tiers # spend-based routing -ucode publish # publish it to the workspace +ucode publish # publish it to the workspace ``` +A managed config carries **agents, models, the default agent, and a tiered spend policy only**. +MCP servers and skills are personal, per-developer configuration (`ucode mcp` / `ucode skills`) and +are never part of it. + `ucode setup` walks through the agents to enable and which one bare `ucode` launches, then per agent: Databricks-hosted models or an external Model Provider Service and the models to expose. Interactive Claude Code and Codex configuration installs gateway-critical values in the OS-managed settings @@ -247,16 +244,14 @@ session. Claude Code is asked one model per family (opus/sonnet/haiku/fable), since it selects models by family alias; any family can be skipped. -The optional sections each edit their own part of the same config, so you can add an MCP server or -change a spend tier later without walking the whole flow. `ucode setup skills --location -main.default,other.schema` skips the prompt. `ucode setup spend-tiers` sets a tiered spend policy -that switches the default agent and model as the workspace burns through a budget. Each section -command also offers to publish right away, so you can apply changes incrementally; answering the -section prompts also runs the matching `ucode configure` step, which does configure this machine. +`ucode setup spend-tiers` edits just its own part of the same config, so you can change a spend tier +later without walking the whole flow. It sets a tiered spend policy that switches the default agent +and model as the workspace burns through a budget, and offers to publish right away so you can apply +changes incrementally. Everything is written to `~/.ucode/managed-state.json` — the one local managed-config file — which -`ucode publish` publishes. Re-running `ucode setup` keeps the MCP servers, skills, tracing table, and -tiered spend policy already authored, rather than clearing them; to drop one, edit the file and reload +`ucode publish` publishes. Re-running `ucode setup` keeps the tracing table and tiered spend policy +already authored, rather than clearing them; to drop one, edit the file and reload it with `ucode setup --from-file`. ```bash @@ -292,7 +287,8 @@ With `-f`/`--file`, `publish` reads a config file produced by `ucode export` and the same validation, diff, and confirmation flow. The file's `workspace` must match the configured workspace (it can never redirect publication elsewhere) and its `spec_version` must be a supported integer; server-owned fields (resource name, workspace ids, timestamps, user ids) and unknown fields -are rejected rather than silently dropped. +are rejected rather than silently dropped. Legacy `mcp_servers`/`skills` fields in an older exported +file are ignored rather than rejected, since the managed config no longer carries them. ### Exporting the config @@ -346,19 +342,18 @@ The output looks like: | `ucode claude --refresh` | Re-check Databricks, refresh models/configuration, and launch Claude Code | | `ucode configure --skip-validate` | Write configs without sending a test message through each agent | | `ucode configure --agents claude,codex,pi --skip-unavailable` | Configure the requested agents that are available; skip the rest with a warning | -| `ucode configure --agents claude --mcp system.ai.slack` | Configure an agent and register its Databricks MCP server(s) in one command | +| `ucode mcp` | Register Databricks MCP servers on your coding tools (interactive picker; replaces the registered set) | +| `ucode mcp --services system.ai.slack` | Register specific MCP service(s) non-interactively (replaces the set) | | `ucode mcp add --location system.ai` | Register a schema's MCP servers, keeping any already configured (additive; never removes) | | `ucode mcp add --services system.ai.slack` | Register specific MCP server(s) without removing existing ones | | `ucode mcp add --agents claude --services system.ai.slack` | Set up the agent(s) if needed and register the server for them | | `ucode mcp remove` | Interactively unregister configured MCP servers from your coding tools | | `ucode mcp remove --agents codex` | Unregister selected servers from specific agents only | -| `ucode configure skills` | Register the skills MCP connection (utility tools only); no skills download | -| `ucode configure skills --location main.default [--path ]` | Download a schema's skills to disk (under ``, or your home dir) and register a schema-less skills MCP connection | -| `ucode configure skills --location main.default --skill my-skill` | Download only the named skill(s) from a schema (comma-separated for several) | -| `ucode configure skills --location main.default --mcp` | Expose a schema's skills as MCP tools (override-only) instead of downloading | +| `ucode skills` | Register the skills MCP connection (utility tools only); no skills download | +| `ucode skills --location main.default [--path ]` | Download a schema's skills to disk (under ``, or your home dir) and register a schema-less skills MCP connection | +| `ucode skills --location main.default --skill my-skill` | Download only the named skill(s) from a schema (comma-separated for several) | +| `ucode skills --location main.default --mcp` | Expose a schema's skills as MCP tools (override-only) instead of downloading | | `ucode setup` | Author the managed config's agents and models (workspace admins only) | -| `ucode setup mcps` | Add or change the managed config's MCP servers | -| `ucode setup skills [--location a.b,c.d]` | Add or change the managed config's skills | | `ucode setup spend-tiers` | Set the managed config's tiered spend routing policy | | `ucode setup help` | Walk through the whole setup sequence, marking what's already configured | | `ucode setup show` | Print the authored config and the payload `ucode publish` would publish | diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 826a6f1f..3da6fb60 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -90,25 +90,21 @@ setup_budget_policy_command, setup_command, setup_help_command, - setup_mcp_command, - setup_skills_command, show_command, ) from ucode.mcp import ( MCP_CLIENTS, SKILLS_MCP_KIND, add_mcp_command, - apply_managed_mcp_servers, - apply_managed_skills, configure_mcp_command, configure_skills_mcp_command, + migrate_off_managed_mcp_and_skills, purge_cross_workspace_mcp_residue, remove_mcp_command, revert_mcp_configs, ) from ucode.skills_download import ( configure_skills_download_command, - download_managed_skills_on_launch, ) from ucode.smart_routing import v2 as smart_routing_v2 from ucode.smart_routing.claude_hooks import FIRST_PROMPT_SOCKET_ENV, ROUTE_FIRST_PROMPT_EVENT @@ -211,23 +207,6 @@ def _print_managed_summary( model = managed_default_model(managed, tool) if model: lines.append(f"[bold]Model:[/bold] [magenta]{model}[/magenta]") - # Always listed, including when empty: "none configured" tells a developer their admin set none, - # which a missing row leaves ambiguous. Shown as the admin configured them — registering them - # locally is a separate change, hence "pending". - mcp_names = [ - str(server.get("name")) - for server in (managed.get("mcp_servers") or []) - if isinstance(server, dict) and server.get("name") - ] - if mcp_names: - lines.append(f"[bold]MCPs:[/bold] {', '.join(mcp_names)} [dim](pending)[/dim]") - else: - lines.append("[bold]MCPs:[/bold] [dim]none configured[/dim]") - skill_names = [str(name) for name in ((managed.get("skills") or {}).get("names") or []) if name] - if skill_names: - lines.append(f"[bold]Skills:[/bold] {', '.join(skill_names)} [dim](pending)[/dim]") - else: - lines.append("[bold]Skills:[/bold] [dim]none configured[/dim]") lines.extend(_policy_summary_lines(managed)) console.print( Panel("\n".join(lines), title="Workspace-managed config", style="green", expand=False) @@ -339,8 +318,8 @@ def _maybe_run_admin_setup(workspace: str, profile: str | None) -> None: return print_note( "You're a workspace admin, and no managed coding agent config exists for this workspace " - "yet — let's set one up. Choose the agents, models, MCPs, and skills once and every " - "developer inherits them when they run `ucode`." + "yet — let's set one up. Choose the agents and models once and every developer " + "inherits them when they run `ucode`." ) _run_setup_and_exit(workspace, profile, token) @@ -1099,12 +1078,8 @@ def status() -> int: print_heading("State") print_kv("State file", str(STATE_PATH) if STATE_PATH.exists() else "missing") print_note("Use `ucode configure` to update workspace settings or configure new tools.") - print_note( - "Use `ucode configure mcp` to add Databricks MCP servers to configured coding tools." - ) - print_note( - "Use `ucode configure skills` to set up Unity Catalog Skills for configured coding tools." - ) + print_note("Use `ucode mcp` to add Databricks MCP servers to configured coding tools.") + print_note("Use `ucode skills` to set up Unity Catalog Skills for configured coding tools.") print_note("Use `ucode configure tracing` to log coding sessions to an MLflow experiment.") print_note("Use `ucode revert` to clear managed configs and restore prior files.") return 0 @@ -1161,8 +1136,8 @@ def revert() -> int: ) configure_app = typer.Typer(add_completion=False, no_args_is_help=False) app.add_typer(configure_app, name="configure", help="Configure workspace and tool settings.") -mcp_app = typer.Typer(add_completion=False, no_args_is_help=True) -app.add_typer(mcp_app, name="mcp", help="MCP servers exposed by ucode.") +mcp_app = typer.Typer(add_completion=False, no_args_is_help=False) +app.add_typer(mcp_app, name="mcp", help="Register Databricks MCP servers on your coding tools.") setup_app = typer.Typer(add_completion=False, no_args_is_help=False) app.add_typer( setup_app, @@ -1254,7 +1229,7 @@ def mcp_add( ) -> None: """Add Databricks MCP servers to installed coding tools. - Like `ucode configure mcp`, but purely additive: it never removes MCP servers + Like `ucode mcp`, but purely additive: it never removes MCP servers that are already configured, only registers new ones. Pass --agents to target (and, if needed, set up) specific agents. """ @@ -1344,7 +1319,7 @@ def mcp_proxy_cmd( """Bridge a coding agent's stdio MCP transport to a Databricks MCP endpoint. Each configured client spawns this as a local stdio MCP server (see - `ucode configure mcp`); it forwards messages to ``--url`` and injects a + `ucode mcp`); it forwards messages to ``--url`` and injects a freshly-minted token on every upstream request, so it never expires mid-session. Not meant for interactive use — the agent manages this process's lifecycle.""" @@ -1765,99 +1740,6 @@ def _print_budget_panel(recommendation: dict, tool: str, managed: dict | None = console.print(panel) -def _register_managed_mcp_servers(managed: dict, tool: str, state: dict) -> None: - """Apply the managed config's MCP servers to ``tool`` and persist what was registered. - - Persisting under ``managed_mcp_servers`` lets the next launch diff against it, so a server the - admin later removes from the config is unregistered rather than left behind. A failure here never - blocks the launch — the agent still starts, just without the workspace's MCP servers. - """ - try: - registered = apply_managed_mcp_servers( - managed, - tool, - state["workspace"], - state.get("profile"), - use_pat=bool(state.get("use_pat")), - ) - except RuntimeError as exc: - print_warning(f"Could not register your workspace's MCP servers: {exc}") - return - # Persist even when empty so a config that dropped its last server clears the prior registration. - others = [ - server - for server in (state.get("managed_mcp_servers") or []) - if isinstance(server, dict) and tool not in (server.get("clients") or []) - ] - state["managed_mcp_servers"] = others + registered - save_state(state) - if registered: - names = ", ".join(str(server["name"]) for server in registered) - print_note(f"Registered workspace MCP server(s) for {TOOL_SPECS[tool]['display']}: {names}") - - -def _managed_skill_locations(managed: dict) -> list[str]: - """The ``.`` skill locations the admin published, or ``[]``.""" - return [ - loc - for loc in ((managed.get("skills") or {}).get("names") or []) - if isinstance(loc, str) and loc - ] - - -def _download_managed_skills(managed: dict, state: dict) -> None: - """Download the admin-published skill schemas to disk (user scope). - - Registering the skills MCP connection (see :func:`_apply_managed_skills`) exposes the skill - *tools* over the gateway, but the agent's ``/skills`` picker reads skill bundles from - ``~/.claude/skills`` / ``~/.agents/skills`` on disk. Without this download those directories stay - empty, so a workspace-published skill never shows up in ``/skills``. Skills already on disk are - left untouched, so a steady-state launch only lists each schema and writes nothing. Best-effort: - a failure here never blocks the launch. - """ - locations = _managed_skill_locations(managed) - if not locations: - return - try: - token = get_databricks_token(state["workspace"], state.get("profile")) - written = download_managed_skills_on_launch(state["workspace"], token, locations) - except RuntimeError as exc: - print_warning(f"Could not download your workspace's skills: {exc}") - return - if written: - print_note(f"Downloaded workspace skill(s) to disk: {', '.join(written)}") - - -def _apply_managed_skills(managed: dict, tool: str, state: dict) -> None: - """Register the managed config's skill schemas on ``tool``'s skills MCP connection and disk. - - Sibling of :func:`_register_managed_mcp_servers` for the skills registry: the managed config - lists the skill schemas the admin published, and nothing else on the launch path routes them to - the agent. ``apply_managed_skills`` persists the connection (and the applied set, for diffing a - later removal) into ``state`` itself, then ``_download_managed_skills`` writes the skill bundles - to disk so the agent's ``/skills`` picker lists them. A failure in either step never blocks the - launch. - """ - try: - applied = apply_managed_skills( - state, - managed, - tool, - state["workspace"], - state.get("profile"), - use_pat=bool(state.get("use_pat")), - ) - except RuntimeError as exc: - print_warning(f"Could not register your workspace's skills: {exc}") - else: - if applied: - names = ", ".join(applied) - print_note( - f"Registered workspace skill schema(s) for {TOOL_SPECS[tool]['display']}: {names}" - ) - _download_managed_skills(managed, state) - - def _can_launch_from_cached_config( tool: str, state: dict, @@ -1938,6 +1820,8 @@ def _launch_tool( if needs_auto_configure: _auto_configure_tool(tool) state = ensure_provider_state(tool) + if not is_dry_run(): + migrate_off_managed_mcp_and_skills(state) # Remembered before the fallback below collapses the two cases: a managed config may not # silently override a provider the user typed on the command line (it errors instead). explicit_provider = provider @@ -2139,13 +2023,6 @@ def _launch_tool( ) if recommendation is not None: _print_budget_panel(recommendation, tool, managed) - # Register the managed config's MCP servers so they reach the agent's `/mcp` list. Nothing - # else on this path does it — the config only lists them — so without this a - # workspace-published server never shows up. `managed` is already None when the config is - # skipped (--skip-managed-config / feature off); --dry-run writes nothing. - if managed is not None and not is_dry_run(): - _register_managed_mcp_servers(managed, tool, state) - _apply_managed_skills(managed, tool, state) if tool == "claude": if smart_routing_v2.enabled(): # Transient launch precedence for the v2 PTY's initial --model flag. @@ -2529,7 +2406,7 @@ def cursor_cmd(ctx: typer.Context) -> None: Cursor is MCP-only: `cursor-agent` runs models on your own Cursor account, so ucode configures no models for it. Its Databricks MCP servers (added via - `ucode configure mcp`) run `ucode mcp-proxy`, which authenticates itself — so + `ucode mcp`) run `ucode mcp-proxy`, which authenticates itself — so this command is a thin convenience wrapper over `cursor-agent`, kept for symmetry with the other `ucode ` launchers. """ @@ -2644,17 +2521,6 @@ def configure( "--disable-databricks-ai-tools to opt out.", ), ] = None, - mcp: Annotated[ - str | None, - typer.Option( - "--mcp", - help="Also register the given Databricks MCP service(s) for the configured " - "coding agents, in one command. Pass a comma-separated list of fully-qualified " - "names like `system.ai.slack`. Combine with --agents to set up an agent and its " - "MCP servers together (e.g. `--agents claude --mcp system.ai.slack`); use without " - "--agents for MCP-only clients such as Cursor.", - ), - ] = None, tracing: Annotated[ bool, typer.Option( @@ -2715,8 +2581,8 @@ def configure( if profiles is not None: workspace_entries = _parse_profiles_option(profiles) # Whether the user named the workspace(s) via flags, captured before the resolver below - # may fill `workspace_entries` from a prompt — this, not the resolved value, decides the - # fully-interactive MCP prompt at the end. + # may fill `workspace_entries` from a prompt — this, not the resolved value, decides + # whether the optional-setup step is offered. flag_driven_workspace = workspace_entries is not None # Under a managed config, resolve (prompting when interactive) and set the target workspace # first, so the developer can switch workspaces; only then short-circuit if that workspace @@ -2741,9 +2607,6 @@ def configure( agent = "claude" if enable_databricks_ai_tools is not None: skip_kwargs["databricks_ai_tools_enabled"] = enable_databricks_ai_tools - # Set True only in the fully-interactive branch below; gates the optional - # MCP setup prompt so flag-driven / scripted runs are never interrupted. - fully_interactive = False combined_optional_setup = False if agent is not None: tool = normalize_tool(agent) @@ -2765,10 +2628,10 @@ def configure( # Cursor is MCP-only (no model routing), so it can't go through the # model-agent configure path. Split it out: model agents configure # normally; cursor only needs workspace state established here, and - # its MCP servers are added separately via `ucode configure mcp` - # (which picks cursor up through MCP_ONLY_CLIENTS). If cursor is the - # only agent, do a workspace-only configure so that later `configure - # mcp` run has a current workspace to target. + # its MCP servers are added separately via `ucode mcp` (which picks + # cursor up through MCP_ONLY_CLIENTS). If cursor is the only agent, do + # a workspace-only configure so a later `ucode mcp` run has a current + # workspace to target. requested = [a.strip().lower() for a in agents.split(",") if a.strip()] wants_cursor = "cursor" in requested model_agent_names = ",".join(a for a in requested if a != "cursor") @@ -2801,19 +2664,6 @@ def configure( else: # Neither model agents nor cursor -> empty/invalid --agents list. _parse_agents_option(agents) - elif mcp is not None: - # MCP-only: `--mcp` without --agent(s) (e.g. Cursor, which isn't a - # model agent, or adding MCP servers to an already-configured setup). - # Configure just the workspace — no interactive agent picker — so the - # `--mcp` registration below has a current workspace to target. - if workspace_entries is None: - workspace_entries = [_prompt_for_configuration(None)] - _configure_shared_workspace_states( - workspace_entries, - tools=[], - force_login=not use_pat, - use_pat=use_pat, - ) else: # Tool binaries are installed after the user picks which agents # they want, in configure_workspace_command. @@ -2833,12 +2683,6 @@ def configure( prompt_optional_updates=prompt_optional_updates, **skip_kwargs, ) - # Only the no-agent, no-workspace path is truly interactive (the user - # picked agents/workspace via prompts); that's where we offer the MCP - # step below. Flag-driven runs stay scriptable. Keyed off whether the - # workspace came from a flag, not the now-resolved `workspace_entries` - # (which the managed-config resolver may have filled from a prompt). - fully_interactive = not flag_driven_workspace if tracing: # The workspaces were just configured, so enable tracing for them # directly instead of re-prompting. Fall back to the workspace that @@ -2849,33 +2693,6 @@ def configure( tracing_workspaces = [(current, None)] if current else None if tracing_workspaces: configure_tracing_command(workspaces=tracing_workspaces) - if mcp is not None: - # The workspace + agents were just configured above, so the current - # workspace state now lists the agents whose MCP configs we should - # write. `--mcp` takes fully-qualified service names, which - # `configure_mcp_command` locates and registers without a picker - # (bare short names would need --location, which we don't accept here). - services = {name.strip() for name in mcp.split(",") if name.strip()} - if not services: - raise RuntimeError( - "--mcp needs at least one fully-qualified MCP service name, e.g. " - "`--mcp system.ai.slack`." - ) - bare = sorted(name for name in services if name.count(".") < 2) - if bare: - raise RuntimeError( - "--mcp names must be fully qualified `..` " - f"(got: {', '.join(bare)}). Use `ucode configure mcp` for the " - "interactive picker." - ) - configure_mcp_command(services=services) - if ( - fully_interactive - and not combined_optional_setup - and not dry_run - and prompt_yes_no("Configure MCP servers now?") - ): - configure_mcp_command() except typer.Exit: # `typer.Exit` subclasses RuntimeError, so it has to be re-raised ahead of the handler # below. Otherwise a clean exit (e.g. `_reject_configure_under_managed_config` under a @@ -2890,8 +2707,9 @@ def configure( raise typer.Exit(130) from None -@configure_app.command("mcp") -def configure_mcp( +@mcp_app.callback(invoke_without_command=True) +def mcp( + ctx: typer.Context, location: Annotated[ str | None, typer.Option( @@ -2914,7 +2732,13 @@ def configure_mcp( ), ] = None, ) -> None: - """Add Databricks MCP servers to installed coding tools.""" + """Add Databricks MCP servers to installed coding tools. + + Bare `ucode mcp` runs the interactive picker (replacing the registered servers with your + selection). Use the `add` / `remove` / `web-search` subcommands for additive or targeted changes. + """ + if ctx.invoked_subcommand is not None: + return # `--services` absent -> None (whole schema); present (even empty) -> the # explicit subset, so `--services ""` deselects everything. selected = None if services is None else {s.strip() for s in services.split(",") if s.strip()} @@ -2928,8 +2752,8 @@ def configure_mcp( raise typer.Exit(130) from None -@configure_app.command("skills") -def configure_skills( +@app.command("skills") +def skills_cmd( location: Annotated[ str | None, typer.Option("--location", help="Comma-separated `.` skill scopes."), @@ -3030,7 +2854,7 @@ def setup( ) -> None: """Choose the agents and models for your workspace's managed config (admins only). - MCP servers, skills, and the tiered spend policy have their own commands — see `ucode setup help`. + The tiered spend policy has its own command — see `ucode setup help`. """ if ctx.invoked_subcommand is not None: return @@ -3049,50 +2873,6 @@ def setup( raise typer.Exit(code) -@setup_app.command("mcps") -def setup_mcp_cmd() -> None: - """Choose the MCP servers the managed config gives developers (admins only).""" - # Same `typer.Exit`/RuntimeError ordering trap as the `setup` callback above. - try: - install_databricks_cli() - code = setup_mcp_command() - 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 - if code: - raise typer.Exit(code) - - -@setup_app.command("skills") -def setup_skills_cmd( - location: Annotated[ - str | None, - typer.Option( - "--location", - help="Skill schemas to publish as `.` (comma-separated for several). " - "Skips the prompt.", - ), - ] = None, -) -> None: - """Choose the skills the managed config gives developers (admins only).""" - try: - install_databricks_cli() - # None means "prompt"; an explicit `--location` is parsed to the list to publish. - locations = None if location is None else _parse_skill_locations(location) - code = setup_skills_command(locations) - 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 - if code: - raise typer.Exit(code) - - @setup_app.command("spend-tiers") def setup_budget_policy_cmd() -> None: """Route developers to cheaper agents as the workspace spends its budget (admins only).""" diff --git a/src/ucode/managed_config.py b/src/ucode/managed_config.py index 7bbf512b..6972af68 100644 --- a/src/ucode/managed_config.py +++ b/src/ucode/managed_config.py @@ -50,8 +50,8 @@ # CodingAgent proto enum -> ucode tool name. Anything unrecognized (e.g. a newer agent this ucode # build doesn't know) is dropped during normalization rather than guessed at. Public because the -# admin-write side (``managed_setup``) inverts these maps to serialize, so a new agent or MCP type -# only has to be declared once. +# admin-write side (``managed_setup``) inverts this map to serialize, so a new agent only has to be +# declared once. AGENT_ENUM_TO_TOOL: dict[str, str] = { "CODING_AGENT_CLAUDE_CODE": "claude", "CODING_AGENT_CODEX": "codex", @@ -61,18 +61,6 @@ "CODING_AGENT_OPENCODE": "opencode", } -# McpServerType proto enum -> ucode's short type tag. Mirrors the selection prefixes in ``mcp.py``; -# the actual name->URL resolution happens there when the manifest is applied (a later change). -MCP_TYPE_ENUM_TO_TAG: dict[str, str] = { - "MCP_SERVER_TYPE_UC_SERVICE": "mcp-service", - "MCP_SERVER_TYPE_EXTERNAL": "external", - "MCP_SERVER_TYPE_GENIE": "genie-space", - "MCP_SERVER_TYPE_VECTOR_SEARCH": "vector-search", - "MCP_SERVER_TYPE_UC_FUNCTIONS": "uc-functions", - "MCP_SERVER_TYPE_DATABRICKS_APP": "app", - "MCP_SERVER_TYPE_DATABRICKS_SQL": "sql", -} - def _as_dict(value: object) -> dict[str, object]: """Return ``value`` as a ``dict[str, object]`` when it is a dict, else an empty dict. @@ -171,19 +159,6 @@ def _tracing_table(tracing: object) -> str | None: return _str(_as_dict(tracing).get("table")) -def _normalize_mcp_servers(value: object) -> list[dict]: - if not isinstance(value, list): - return [] - out: list[dict] = [] - for entry in value: - entry_dict = _as_dict(entry) - name = _str(entry_dict.get("name")) - tag = MCP_TYPE_ENUM_TO_TAG.get(_str(entry_dict.get("type")) or "") - if name and tag: - out.append({"name": name, "type": tag}) - return out - - def _normalize_budget_policy(value: object) -> dict | None: bp = _as_dict(value) if not bp: @@ -218,8 +193,9 @@ def _normalize_budget_policy(value: object) -> dict | None: def normalize_managed_config(raw: dict) -> dict: """Normalize a raw ``CodingAgentConfig`` proto-JSON dict into ucode's internal shape. - The internal shape uses ucode's own tool names and short MCP type tags so downstream reconcile - and apply code never touches proto enum spellings. Unknown agents / MCP types are dropped. + The internal shape uses ucode's own tool names so downstream reconcile and apply code never + touches proto enum spellings. Unknown agents are dropped. MCP servers and skills are personal + configuration and are deliberately not read from the manifest. """ raw = _as_dict(raw) result: dict = {} @@ -241,12 +217,6 @@ def normalize_managed_config(raw: dict) -> dict: enabled_agents[tool] = agent_config if enabled_agents: result["enabled_agents"] = enabled_agents - mcp_servers = _normalize_mcp_servers(raw.get("mcp_servers")) - if mcp_servers: - result["mcp_servers"] = mcp_servers - skill_names = _str_list(_as_dict(raw.get("skills")).get("names")) - if skill_names: - result["skills"] = {"names": skill_names} tracing_table = _tracing_table(raw.get("tracing")) if tracing_table: result["tracing_table"] = tracing_table diff --git a/src/ucode/managed_publish.py b/src/ucode/managed_publish.py index 33f5f623..c370aa2d 100644 --- a/src/ucode/managed_publish.py +++ b/src/ucode/managed_publish.py @@ -24,6 +24,8 @@ _ENVELOPE_FIELDS = ("workspace", "spec_version") +_LEGACY_IGNORED_FIELDS = ("mcp_servers", "skills") + def load_publish_payload(file_path: str | None) -> dict: """Return the source config dict for ``ucode publish``. @@ -71,7 +73,8 @@ def parse_publish_payload(payload: object, workspace: str) -> tuple[dict, dict]: normalizes to the configured one (the file can never redirect publication elsewhere); a ``spec_version`` that is a JSON integer (not a boolean or float) equal to the supported version; no server-owned ``name``; and no unknown or lossy fields (anything normalization would silently - drop is rejected instead). + drop is rejected instead). Legacy ``mcp_servers`` / ``skills`` are the one exception — the managed + config no longer owns them, so they are dropped silently rather than rejected. """ if not isinstance(payload, dict): raise RuntimeError(f"The config must be a JSON object, not a {type(payload).__name__}.") @@ -102,7 +105,11 @@ def parse_publish_payload(payload: object, workspace: str) -> tuple[dict, dict]: f"{EXPORT_SPEC_VERSION}. Upgrade ucode, or re-export the config." ) - config = {key: value for key, value in payload.items() if key not in _ENVELOPE_FIELDS} + config = { + key: value + for key, value in payload.items() + if key not in _ENVELOPE_FIELDS and key not in _LEGACY_IGNORED_FIELDS + } if "name" in config: raise RuntimeError( 'The config includes a server-owned "name" field. Remove it — the workspace assigns the ' diff --git a/src/ucode/managed_setup.py b/src/ucode/managed_setup.py index 3c52f909..94c3ca5e 100644 --- a/src/ucode/managed_setup.py +++ b/src/ucode/managed_setup.py @@ -10,8 +10,9 @@ The manifest shape here is exactly the one :func:`ucode.managed_config.normalize_managed_config` produces, so ``serialize`` then ``normalize`` round-trips to the input. The enum maps are derived by -inverting that module's maps rather than restated, so a new agent or MCP type only has to be added -once. +inverting that module's maps rather than restated, so a new agent only has to be added once. The +managed config carries agents/models/global policy/spend tiers only — MCP servers, skills, and +tracing are personal configuration and are not part of this manifest. Local persistence is not duplicated here: the authored manifest is saved to and loaded from the one local file, ``~/.ucode/managed-state.json``, via :func:`ucode.managed_config.save_managed_state` and @@ -33,14 +34,9 @@ ) from ucode.managed_config import ( AGENT_ENUM_TO_TOOL, - MCP_TYPE_ENUM_TO_TAG, ) -# ucode tool name -> CodingAgent proto enum, and ucode MCP type tag -> McpServerType proto enum. -# Inverted from the read side's maps so the two directions cannot drift: adding an agent to -# `managed_config._AGENT_ENUM_TO_TOOL` makes it serializable here automatically. AGENT_TOOL_TO_ENUM: dict[str, str] = {tool: enum for enum, tool in AGENT_ENUM_TO_TOOL.items()} -MCP_TAG_TO_TYPE_ENUM: dict[str, str] = {tag: enum for enum, tag in MCP_TYPE_ENUM_TO_TAG.items()} # Agents whose model config carries a flat `models` list. Claude instead uses per-family slots # (`ClaudeDefaultModels`), and Codex has no model list at all — it selects exactly one model. @@ -282,9 +278,10 @@ def serialize_managed_config(manifest: dict) -> dict: """Serialize ucode's internal manifest into a proto-JSON ``CodingAgentConfig``. The exact inverse of :func:`ucode.managed_config.normalize_managed_config`: tool names become - ``CODING_AGENT_*`` enums, MCP type tags become ``MCP_SERVER_TYPE_*``, and each agent's model - config is wrapped in its matching ``AgentModelConfig`` oneof variant. Agents and MCP types this - build doesn't recognize are dropped, mirroring the read side. + ``CODING_AGENT_*`` enums and each agent's model config is wrapped in its matching + ``AgentModelConfig`` oneof variant. Agents this build doesn't recognize are dropped, mirroring the + read side. The manifest carries agents/models/default-agent/tracing/budget policy — MCP servers + and skills are not serialized. Output-only proto fields (``workspace_id``, timestamps, user ids) are never emitted. ``name`` is carried through when present so an update path can address an existing resource; ``ucode publish`` @@ -313,27 +310,6 @@ def serialize_managed_config(manifest: dict) -> dict: if entries: payload["enabled_agents"] = entries - mcp_servers = manifest.get("mcp_servers") - if isinstance(mcp_servers, list): - servers: list[dict] = [] - for server in mcp_servers: - if not isinstance(server, dict): - continue - server_name = server.get("name") - type_enum = MCP_TAG_TO_TYPE_ENUM.get(str(server.get("type") or "")) - if isinstance(server_name, str) and server_name and type_enum: - servers.append({"name": server_name, "type": type_enum}) - if servers: - payload["mcp_servers"] = servers - - skills = manifest.get("skills") - if isinstance(skills, dict): - names = skills.get("names") - if isinstance(names, list): - skill_names = [n for n in names if isinstance(n, str) and n] - if skill_names: - payload["skills"] = {"names": skill_names} - tracing_table = manifest.get("tracing_table") if isinstance(tracing_table, str) and tracing_table: payload["tracing"] = {"table": tracing_table} @@ -422,7 +398,6 @@ def validate_manifest(manifest: dict, state: dict | None = None) -> list[str]: - ``default_agent`` is required once any agent configuration is present, must appear in ``enabled_agents``, and that agent must have a non-empty ``default_model``; - every ``enabled_agents`` key must be an agent this ucode build knows; - - each MCP server needs a name and a recognized type; skill names must be non-empty; - ``tracing_table`` must be non-empty when the key is present; - a ``budget_policy`` needs a ``budget_id``, and each tier needs a ``spending_percentage`` in [0, 1] (unique across tiers), a ``default_agent`` that appears in ``enabled_agents``, and a @@ -467,29 +442,6 @@ def validate_manifest(manifest: dict, state: dict | None = None) -> list[str]: for tool, agent_config in enabled_agents.items(): errors.extend(_validate_agent_models(tool, agent_config, known)) - mcp_servers = manifest.get("mcp_servers") - if isinstance(mcp_servers, list): - for index, raw_server in enumerate(mcp_servers, start=1): - if not isinstance(raw_server, dict): - errors.append(f"mcp_servers[{index}] must be an object.") - continue - server = _as_dict(raw_server) - if not server.get("name"): - errors.append(f"mcp_servers[{index}]: name is required.") - server_type = str(server.get("type") or "") - if server_type not in MCP_TAG_TO_TYPE_ENUM: - valid = ", ".join(sorted(MCP_TAG_TO_TYPE_ENUM)) - errors.append( - f"mcp_servers[{index}]: type '{server_type}' is not recognized " - f"(valid: {valid})." - ) - - skills = manifest.get("skills") - if isinstance(skills, dict): - names = skills.get("names") - if isinstance(names, list) and any(not isinstance(name, str) or not name for name in names): - errors.append("skills.names must not contain empty names.") - if "tracing_table" in manifest and not manifest.get("tracing_table"): errors.append("tracing_table must not be empty.") diff --git a/src/ucode/managed_wizard.py b/src/ucode/managed_wizard.py index 6854f833..6dddbb4d 100644 --- a/src/ucode/managed_wizard.py +++ b/src/ucode/managed_wizard.py @@ -6,14 +6,14 @@ :mod:`ucode.managed_config`). Authoring is split across commands so an admin can change one part without walking the whole flow: -``ucode setup`` picks the agents and models, and ``ucode setup mcps`` / ``skills`` / ``spend-tiers`` -each edit their own section of the same manifest. ``ucode setup`` carries the other sections forward -untouched (:func:`_carry_forward_sections`), and ``ucode setup help`` prints the whole sequence. +``ucode setup`` picks the agents and models, and ``ucode setup spend-tiers`` edits the tiered spend +policy of the same manifest. ``ucode setup`` carries the other sections forward untouched +(:func:`_carry_forward_sections`), and ``ucode setup help`` prints the whole sequence. The managed +config carries agents/models/global policy/spend tiers only — MCP servers and skills are personal +configuration (`ucode mcp` / `ucode skills`), not part of it. Serialization, validation, and the per-agent model catalogs live in :mod:`ucode.managed_setup`; this -module is the interaction layer on top of them. Sub-flows an admin already knows — MCP, skills — are -delegated to the existing ``ucode configure `` commands and their results read back out of -``state.json``, so there is exactly one picker per concern in the codebase. +module is the interaction layer on top of them. """ from __future__ import annotations @@ -114,89 +114,6 @@ def _tracing_table_from_state(state: dict) -> str | None: return destination if isinstance(destination, str) and destination else None -def _mcp_server_from_url(url: str) -> tuple[str, str] | None: - """Derive a managed-config ``(name, type)`` entry from a registered server's resolved URL. - - ``state.json`` stores each MCP server's resolved URL but not its type, while the managed config - stores ``{name, type}`` and lets the developer's ucode rebuild the URL. So map the URL back to the - type *and* the identifier the ai-gateway ``McpServer.name`` field is meant to hold for that type - (a UC name for a UC service, a Genie space id for a genie space, a `.` for - vector-search / uc-functions, a connection name for external). Deriving ``name`` from the URL — - rather than reusing the local display slug — is what lets the developer's ucode reconstruct the - URL on launch. Returns None for a URL that matches nothing reconstructable (e.g. an app's - off-workspace host), so those are skipped rather than published unusably. - """ - stripped = url.rstrip("/") - marker = "/ai-gateway/mcp-services/" - if marker in url: - # `.../mcp-services/..` — store the dash form the launch path expects. - service = url.split(marker, 1)[1].split("/", 1)[0] - return service.replace(".", "-"), "mcp-service" - for fragment, tag in ( - ("/api/2.0/mcp/external/", "external"), - ("/api/2.0/mcp/genie/", "genie-space"), - ): - if fragment in url: - # external -> connection name; genie -> space id. Both are the single trailing segment. - return url.split(fragment, 1)[1].split("/", 1)[0], tag - for fragment, tag in ( - ("/api/2.0/mcp/vector-search/", "vector-search"), - ("/api/2.0/mcp/functions/", "uc-functions"), - ): - if fragment in url: - # `...//` — store the `.` the launch path splits back. - rest = url.split(fragment, 1)[1].split("/") - if len(rest) >= 2 and rest[0] and rest[1]: - return f"{rest[0]}.{rest[1]}", tag - return None - if stripped.endswith("/api/2.0/mcp/sql"): - return "databricks-sql", "sql" - # Databricks apps are the residual case: an arbitrary app host with a /mcp suffix. Its host isn't - # reconstructable from the workspace + an id, so it can't be published to the managed config yet. - if stripped.endswith("/mcp"): - return None - return None - - -def _mcp_servers_from_state(state: dict) -> list[dict]: - """The registered MCP servers, as managed-config ``{name, type}`` entries. - - Skips the skills registry connection: skills are published under the manifest's own ``skills`` - field, so including its MCP entry would configure it twice. - """ - from ucode.mcp import SKILLS_MCP_KIND - - servers: list[dict] = [] - seen: set[str] = set() - for entry in state.get("mcp_servers") or []: - if not isinstance(entry, dict) or entry.get("kind") == SKILLS_MCP_KIND: - continue - name = entry.get("name") - url = entry.get("url") - if not isinstance(name, str) or not name or not isinstance(url, str): - continue - resolved = _mcp_server_from_url(url) - if resolved is None: - print_warning( - f"Skipping MCP server '{name}': ucode can't publish it to a managed config " - f"(unrecognized or app-hosted URL: {url})." - ) - continue - config_name, tag = resolved - if config_name in seen: - continue - seen.add(config_name) - servers.append({"name": config_name, "type": tag}) - return servers - - -def _skill_names_from_state(state: dict) -> list[str]: - """Skill schemas registered on the skills MCP connection (``catalog.schema`` entries).""" - from ucode.mcp import _skill_mcp_locations - - return [name for name in _skill_mcp_locations(state) if isinstance(name, str) and name] - - def provider_service_model_options(service: dict) -> list[str]: """Model ids an admin can pick from a provider service, or [] when they can't be enumerated. @@ -1085,15 +1002,6 @@ def _render_summary(workspace: str, manifest: dict) -> None: elif isinstance(models, list) and len(models) > 1: lines.append(kv_line(" models", ", ".join(str(m) for m in models))) - mcp_servers = manifest.get("mcp_servers") or [] - lines.append( - kv_line( - "MCP servers", - ", ".join(str(server.get("name")) for server in mcp_servers) if mcp_servers else "none", - ) - ) - skills = (manifest.get("skills") or {}).get("names") or [] - lines.append(kv_line("Skills", ", ".join(skills) if skills else "none")) # Managed tracing isn't offered by the flow yet, so a "disabled" line is just noise. Only surface # it when a `--from-file` config actually set a table. if manifest.get("tracing_table"): @@ -1121,7 +1029,7 @@ def _config_facts(manifest: dict) -> list[tuple[str, str, str]]: """Flatten a normalized config into ordered ``(key, label, value)`` facts, for diffing. Each fact is one thing an admin would think of as a single setting — the default agent, an agent's - model, its settings scope, an MCP server, a skill, the tracing table, a budget tier. The ``key`` is + model, its settings scope, the tracing table, a budget tier. The ``key`` is a stable identity so the same setting lines up across two configs even when values differ; the ``label`` is what the admin reads. Deliberately mirrors what :func:`_render_summary` chooses to show, so the diff and the summary never disagree about what's in a config. @@ -1152,13 +1060,6 @@ def _config_facts(manifest: dict) -> list[tuple[str, str, str]]: facts.append((f"agent:{tool}:model:{family}", f"{display} ({family})", str(model))) elif isinstance(models, list) and len(models) > 1: facts.append((f"agent:{tool}:models", f"{display} models", ", ".join(map(str, models)))) - for server in manifest.get("mcp_servers") or []: - name = str(server.get("name")) - facts.append((f"mcp:{name}", f"MCP server {name}", str(server.get("type") or ""))) - - for skill in (manifest.get("skills") or {}).get("names") or []: - facts.append((f"skill:{skill}", f"Skill {skill}", "published")) - tracing = manifest.get("tracing_table") if tracing: facts.append(("tracing_table", "Tracing table", str(tracing))) @@ -1255,8 +1156,8 @@ def _handle_existing_config(workspace: str, token: str) -> tuple[bool, dict | No Returns ``(keep_going, existing)``: ``keep_going`` is True to continue authoring (publishing later replaces the existing config) and False to stop (the admin chose to delete it instead). ``existing`` - is the published config when one was read, so the caller can carry its MCP servers / skills / - tracing / budget policy forward — the local draft may be missing on a fresh machine or after + is the published config when one was read, so the caller can carry its tracing table and budget + policy forward — the local draft may be missing on a fresh machine or after ``ucode revert``, and without this those sections would be silently dropped on the next publish. Deliberately doesn't itemize what the existing config holds. The admin doesn't need an inventory @@ -1275,8 +1176,8 @@ def _handle_existing_config(workspace: str, token: str) -> tuple[bool, dict | No return True, None print_warning( - "This workspace already has a managed configuration — one config covers every agent, MCP " - "server, skill, tracing table, and budget policy for the whole workspace." + "This workspace already has a managed configuration — one config covers every agent, " + "tracing table, and budget policy for the whole workspace." ) choice = prompt_for_selection( "What would you like to do?", @@ -1376,8 +1277,6 @@ def setup_from_file(path: str) -> int: # The sections that have their own `ucode setup ` command, in the order the checklist lists # them: the command, the label the summary uses, and how to tell whether the manifest has one. SETUP_SECTIONS: list[tuple[str, str, Callable[[dict], bool]]] = [ - ("ucode setup mcps", "MCP servers", lambda m: bool(m.get("mcp_servers"))), - ("ucode setup skills", "Skills", lambda m: bool((m.get("skills") or {}).get("names"))), ( "ucode setup spend-tiers", "Tiered Spend Policy", @@ -1481,8 +1380,6 @@ def _offer_publish() -> None: # The sections `ucode setup` carries forward instead of prompting for, and how to rebuild each one. CARRIED_SECTIONS: list[tuple[str, str, str]] = [ - ("mcp_servers", "MCP servers", "ucode setup mcps"), - ("skills", "Skills", "ucode setup skills"), ("tracing_table", "Tracing table", "ucode setup --from-file"), ("budget_policy", "Tiered Spend Policy", "ucode setup spend-tiers"), ] @@ -1491,9 +1388,9 @@ def _offer_publish() -> None: def _carry_forward_sections(previous: dict, manifest: dict) -> None: """Copy the sections `ucode setup` no longer prompts for out of a previously authored config. - `setup` writes the whole manifest, so without this a re-run would silently clear the MCP servers, - skills, tracing table, and budget policy an admin authored with the other commands — they'd have - to redo every one of them just to change a model. + `setup` writes the whole manifest, so without this a re-run would silently clear the tracing + table and budget policy an admin authored with the other commands — they'd have to redo both + just to change a model. Each section is probe-validated before it's carried, and dropped with a warning if it no longer fits. Otherwise a carried section could make the manifest invalid and block the save outright, @@ -1535,9 +1432,8 @@ def setup_command( ) -> int: """Author the agents and models half of the workspace's managed coding config interactively. - Agents and per-agent models only. MCP servers, skills, and the tiered spend policy each have their - own command (`ucode setup mcps` / `skills` / `spend-tiers`), so an admin changing one of them doesn't - have to walk the whole flow again — and this command carries whatever they already authored + Agents and per-agent models only. The tiered spend policy has its own command (`ucode setup + spend-tiers`), so an admin changing it doesn't have to walk the whole flow again — and this command carries whatever they already authored forward untouched rather than clearing it (:func:`_carry_forward_sections`). ``workspace``/``profile`` let a caller that has already resolved (and authenticated against) a @@ -1547,7 +1443,7 @@ def setup_command( ``command_label`` brands the section headers to the invoking command: `ucode configure` passes "Configure Unity Gateway" so a user who never typed `ucode setup` isn't jarred by it (the standalone `ucode setup` command keeps the default). References to specific sub-commands (`ucode - setup mcps`, `ucode apply`, …) stay verbatim — those are real command names, not branding. + setup spend-tiers`, `ucode apply`, …) stay verbatim — those are real command names, not branding. ``token`` lets a caller that already authenticated and admin-checked the workspace (e.g. `ucode configure`) hand its token in, so setup's admin gate uses the *same* token as the routing @@ -1602,7 +1498,7 @@ def setup_command( # The local draft is the carry-forward source, falling back to what's published on the workspace: # a fresh machine (or one after `ucode revert`) has no draft, and without the fallback the next - # publish would silently wipe the workspace's MCP servers, skills, tracing, and budget policy. + # publish would silently wipe the workspace's tracing table and budget policy. previous = load_managed_state(workspace) or published or {} previously_enabled = [ tool for tool in (previous.get("enabled_agents") or {}) if tool in available @@ -1675,9 +1571,8 @@ def _resolve_admin_workspace() -> tuple[str, str | None, str]: """Resolve the workspace a section command edits, authenticate, and gate on admin. Returns ``(workspace, profile, token)``. Unlike `ucode setup`, this doesn't prompt for a workspace - and takes it strictly from local state rather than falling back to the draft file's workspace: the - MCP and skills pickers re-read ``current_workspace`` themselves (via ``setup_mcp_clients``), so a - mismatch would have them operate against one workspace while the manifest is saved for another. + and takes it strictly from local state rather than falling back to the draft file's workspace, so a + mismatch can't have the section saved for one workspace while local state points at another. Requiring ``ucode configure`` to have set the current workspace keeps the two in lockstep. It also skips :func:`_handle_existing_config` — the create-or-delete choice belongs to authoring a config, not to changing one section of it. @@ -1739,84 +1634,6 @@ def _save_section_update(workspace: str, manifest: dict) -> int: return 0 -def setup_mcp_command() -> int: - """Author the managed config's MCP servers (`ucode setup mcps`).""" - workspace, _, _ = _resolve_admin_workspace() - manifest = _manifest_for_edit(workspace) - - print_section("Managed MCP servers") - print_note("Developers get these MCP servers registered automatically when they run ucode.") - from ucode.mcp import configure_mcp_command - - # Snapshot the managed-shaped servers before the picker so a cancelled run on an empty local - # state can't delete a section the manifest still has: the picker returns 0 on Esc, and re-reading - # local state would otherwise overwrite the manifest with nothing. - before = _mcp_servers_from_state(load_state()) - # Managed configs can't carry a Databricks app (its host isn't reconstructable from the - # workspace), so hide apps from the picker rather than let an admin pick one that is then - # dropped from the published config. - configure_mcp_command(exclude_sources={"apps"}) - after = _mcp_servers_from_state(load_state()) - - # `after == before` isn't enough to call this a no-op: an admin who ran `ucode configure mcp` - # first arrives with those servers already registered, so confirming the picker leaves local state - # unchanged even though the manifest doesn't carry them yet. Also sync when local state already - # holds servers the manifest is missing — but only when servers are actually registered, so an Esc - # on an empty local state still can't wipe a published section. - manifest_servers = manifest.get("mcp_servers") or [] - carries_unsaved = bool(after) and after != manifest_servers - if after == before and not carries_unsaved: - print_note("No changes to the MCP servers — the managed config is unchanged.") - return 0 - - if after: - manifest["mcp_servers"] = after - print_success(f"{len(after)} MCP server(s) in the managed config") - else: - # Deregistering every server locally is how an admin clears the section — there is no - # separate "remove them all" flag. - manifest.pop("mcp_servers", None) - print_note("No MCP servers are registered, so the managed config now carries none.") - return _save_section_update(workspace, manifest) - - -def setup_skills_command(locations: list[str] | None = None) -> int: - """Author the managed config's skills (`ucode setup skills`). - - ``locations`` comes from ``--location`` (already parsed to `.` refs); when None - the admin is prompted and the answer is parsed the same way. - """ - workspace, _, _ = _resolve_admin_workspace() - manifest = _manifest_for_edit(workspace) - - print_section("Managed skills") - print_note("Developers get these skills downloaded automatically when they run ucode.") - if locations is None: - answer = prompt_for_text( - "Skill schemas to publish, comma-separated `catalog.schema` (blank to leave unchanged)", - default="", - ) - # Route the interactive answer through the same parser as `--location` so `main` (missing the - # schema) is rejected here rather than published as a bogus skill name. - from ucode.cli import _parse_skill_locations - - locations = _parse_skill_locations(answer) - # A blank answer / empty `--location` means "leave the skills alone". Returning before delegating - # matters: `configure_skills_mcp_command([])` is not a no-op — it registers the schema-less skills - # MCP connection into the admin's own agents. - if not locations: - print_note("No skill schemas given — the managed config's skills are unchanged.") - return 0 - - from ucode.mcp import configure_skills_mcp_command - - configure_skills_mcp_command(locations) - skill_names = _skill_names_from_state(load_state()) or locations - manifest["skills"] = {"names": skill_names} - print_success(f"{len(skill_names)} skill schema(s) in the managed config") - return _save_section_update(workspace, manifest) - - def setup_budget_policy_command() -> int: """Author the managed config's tiered spend policy (`ucode setup spend-tiers`).""" workspace, _, token = _resolve_admin_workspace() @@ -1857,7 +1674,7 @@ def setup_help_command() -> int: print_section("ucode setup") print_note( "A managed config is the coding setup your developers pull automatically — they run ucode " - "and get the agents, models, MCP servers, and skills you chose here. Admins only." + "and get the agents and models you chose here. Admins only." ) print_note( "Each command below edits your local draft; nothing reaches the workspace until " @@ -2097,7 +1914,5 @@ def publish_command(*, file_path: str | None = None, yes: bool = False) -> int: "setup_command", "setup_from_file", "setup_help_command", - "setup_mcp_command", - "setup_skills_command", "show_command", ] diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 75e3699d..885ff63d 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -888,7 +888,7 @@ def build_mcp_picker_choices( def known_choice(name: str, title: str | None = None) -> questionary.Choice: # `ucode mcp add` (additive) never removes an already-configured server, so # show it as a non-toggleable note rather than a pre-checked box whose - # unchecking would be silently ignored. `configure mcp` (replace) keeps it a + # unchecking would be silently ignored. `ucode mcp` (replace) keeps it a # pre-checked toggle so unchecking removes it. if additive: return questionary.Choice( @@ -1052,7 +1052,7 @@ def _is_app_mcp_server(server: dict) -> bool: Apps are the residual ``/mcp`` URL shape — everything else ucode registers is a known workspace-relative path. Used to hide already-registered apps from the picker where they can't be - published (``ucode setup``).""" + published to a managed config.""" url = server.get("url") if not isinstance(url, str): return False @@ -1071,162 +1071,83 @@ def _is_app_mcp_server(server: dict) -> bool: return stripped.endswith("/mcp") -def managed_mcp_server_entry(name: str, mcp_type: str, workspace: str) -> tuple[str, str] | None: - """Rebuild an ``(entry_name, url)`` pair from a managed config's ``{name, type}`` entry. +def migrate_off_managed_mcp_and_skills(state: dict) -> bool: + """One-time cleanup of MCP servers / skill schemas a prior ucode applied from a managed config. - ``entry_name`` is the identifier the server is registered under with the agent (dots stripped, - since the agent CLIs reject them); ``url`` is what the proxy forwards to. Returns None for a - type/name this can't reconstruct, so the caller skips it rather than registering a broken server. - Mirrors the shapes :func:`_resolve_mcp_selection` builds for the interactive picker, so a managed - and a locally-configured copy of the same server land on the same name. + Managed configs no longer carry MCP servers or skills, so any registrations a prior version + applied — tracked under ``managed_mcp_servers`` / ``managed_skill_locations`` — are undone here: + those MCP servers are unregistered from the agents they were registered on, and the managed skill + schemas are dropped from the skills connection while the developer's own schemas are kept. This is + the reverse of the old apply path, reusing the same reconcile primitive. The markers are the only + record of what was registered: the old apply path kept managed servers out of ``mcp_servers``, so + diffing that list would find nothing to undo. Downloaded skill *files* + are left on disk: they carry no origin marker, so deleting them risks removing the developer's own + files — remove them by hand if unwanted. - The ai-gateway ``McpServer.name`` field is interpreted per ``type`` (see the proto): a UC name for - a UC service, a Genie space id for a genie space, a connection name for external, and — as ucode - serializes them — a `.` for vector-search / uc-functions. + Best-effort and idempotent: a no-op returning False when there are no markers (or no workspace to + reconcile against), and on any failure it warns, keeps the markers, and returns False so a later + run retries. Returns True when it unregistered something and cleared the markers. """ - if mcp_type == "sql": - return "databricks-sql", f"{workspace}/api/2.0/mcp/sql" - if mcp_type == "external": - return name, f"{workspace}/api/2.0/mcp/external/{name}" - if mcp_type == "mcp-service": - # Stored in dash form (`system-ai-dbsql`), which is already the registered name; the URL wants - # the UC dotted form. Only the catalog and schema separators (first two dashes) become dots — - # the service name keeps its own dashes/underscores. - parts = name.split("-", 2) - if len(parts) != 3: - return None - return name, build_mcp_service_url(workspace, ".".join(parts)) - if mcp_type == "genie-space": - # `name` is the Genie space id (per the proto); register under the id-based name the - # interactive path falls back to, and point the URL at the space. - return f"databricks-genie-{name}", f"{workspace}/api/2.0/mcp/genie/{name}" - if mcp_type in ("vector-search", "uc-functions"): - # `name` is a `.`; the URL is workspace-relative on that pair, and the - # registered name is the same dot-free slug the interactive path uses. - catalog, _, schema = name.partition(".") - if not catalog or not schema or "." in schema: - return None - url_path = "vector-search" if mcp_type == "vector-search" else "functions" - name_prefix = ( - "databricks-vector-search" if mcp_type == "vector-search" else "databricks-functions" - ) - entry_name = _catalog_schema_server_name(name_prefix, catalog, schema, set()) - return entry_name, f"{workspace}/api/2.0/mcp/{url_path}/{catalog}/{schema}" - return None - - -def apply_managed_mcp_servers( - managed: dict, tool: str, workspace: str, profile: str | None = None, *, use_pat: bool = False -) -> list[dict]: - """Register the managed config's MCP servers with ``tool`` so they reach its `/mcp` list. - - The managed config only lists ``{name, type}`` entries; nothing else on the launch path turns - them into agent MCP registrations, so without this a workspace-published server never shows up. - Reconstructs each entry's ``(name, url)`` (see :func:`managed_mcp_server_entry`), diffs against - what ucode previously registered, and applies the change for the launching tool only. Entries - whose URL can't be rebuilt (e.g. ``app``, which needs an off-workspace host) are skipped. - - Returns the server dicts registered (for state persistence); an empty list when the config names - none, or names only types that can't yet be reconstructed. - """ - if tool not in MCP_CLIENTS: - return [] - entries = managed.get("mcp_servers") - if not isinstance(entries, list): - return [] - working: list[dict] = [] - seen: set[str] = set() - skipped: list[str] = [] - for entry in entries: - if not isinstance(entry, dict): - continue - name = entry.get("name") - mcp_type = entry.get("type") - if not isinstance(name, str) or not name or not isinstance(mcp_type, str): - continue - resolved = managed_mcp_server_entry(name, mcp_type, workspace) - if resolved is None: - skipped.append(f"{name} ({mcp_type})") - continue - entry_name, url = resolved - if entry_name in seen: - continue - seen.add(entry_name) - working.append({"name": entry_name, "url": url, "auth": "proxy", "clients": [tool]}) - if skipped: - print_warning( - "Skipping managed MCP server(s) ucode can't yet auto-register from the workspace " - f"config: {', '.join(skipped)}. Add them with `ucode configure mcp`." - ) - if not working: - return [] - # Diff against the managed servers ucode registered on a prior launch so a removed entry is - # unregistered and an unchanged one is a no-op. Only this tool's managed servers are considered. - state = load_state() - previous = [ - server - for server in (state.get("managed_mcp_servers") or []) - if isinstance(server, dict) and tool in (server.get("clients") or []) - ] - apply_mcp_server_changes(previous, working, [tool], workspace, profile, use_pat=use_pat) - return working - - -def apply_managed_skills( - state: dict, - managed: dict, - tool: str, - workspace: str, - profile: str | None = None, - *, - use_pat: bool = False, -) -> list[str]: - """Register the managed config's skill schemas on ``tool``'s skills MCP connection. - - The managed config lists skill schemas the admin published as ``catalog.schema`` locations under - ``skills.names``; nothing else on the launch path routes them to the agent, so without this a - workspace-published skill schema never reaches the agent's skills registry. Merges them into the - developer's own ``skill_locations`` (preserving those), diffs against what ucode applied on a - prior launch — tracked under ``managed_skill_locations`` — so a schema the admin later drops is - removed, and registers the single skills connection for the launching tool. - - Mutates and persists ``state`` in place (the skills connection lives in ``state['mcp_servers']``). - Returns the managed locations applied (for the caller's note), or ``[]`` when nothing changed — - the config names none and none were applied before, or the connection was already current. - - When the admin drops their last schema and the developer configured none of their own, the - connection is rebuilt with no ``skill_locations`` (the schema-less, utility-only form), matching - ``configure skills --mcp`` with no location, rather than being removed outright. - """ - if tool not in MCP_CLIENTS: - return [] - desired = [ - loc - for loc in ((managed.get("skills") or {}).get("names") or []) - if isinstance(loc, str) and loc - ] - prev_managed = [ + managed_servers = [s for s in (state.get("managed_mcp_servers") or []) if isinstance(s, dict)] + managed_locations = [ loc for loc in (state.get("managed_skill_locations") or []) if isinstance(loc, str) and loc ] - if not desired and not prev_managed: - return [] - # Preserve the developer's own locations, drop previously-managed ones no longer in the config, - # and add the current managed set. dict.fromkeys dedupes while keeping first-seen order. - current = _skill_mcp_locations(state) - developer_own = [loc for loc in current if loc not in prev_managed] - new_locations = list(dict.fromkeys([*developer_own, *desired])) - - original = list(state.get("mcp_servers") or []) - working = _resolve_skills_mcp_servers(workspace, [tool], new_locations, original) - changed = apply_mcp_server_changes( - original, working, [tool], workspace, profile, use_pat=use_pat - ) - if not (changed or original != working or prev_managed != desired): - return [] - state["mcp_servers"] = working - state["managed_skill_locations"] = desired + if not managed_servers and not managed_locations: + return False + workspace = state.get("workspace") + if not workspace: + return False + profile = state.get("profile") + use_pat = bool(state.get("use_pat")) + # A marker outlives the agent it names, and unregistering shells out to that agent's CLI, so an + # agent since uninstalled is skipped: there is nothing left to unregister from. + installed = set(available_mcp_clients()) + try: + if managed_servers: + # Markers repeat a name once per client; the reconcile primitive keys by name and reads + # each server's own clients, so collapse duplicates into one marker per name first. + merged: dict[str, dict] = {} + for server in managed_servers: + name = _server_name(server) + if not name: + continue + entry = merged.setdefault(name, {**server, "clients": []}) + entry["clients"] = sorted( + {*entry["clients"], *(server.get("clients") or [])} & installed + ) + clients = sorted({c for server in merged.values() for c in server["clients"]}) + if clients: + apply_mcp_server_changes( + list(merged.values()), [], clients, workspace, profile, use_pat=use_pat + ) + if managed_locations: + current_locs = _skill_mcp_locations(state) + developer_locs = [loc for loc in current_locs if loc not in managed_locations] + if developer_locs != current_locs: + skills_entry = next( + iter(_skills_entries(list(state.get("mcp_servers") or []))), None + ) + clients = [c for c in ((skills_entry or {}).get("clients") or []) if c in installed] + original = list(state.get("mcp_servers") or []) + working = _resolve_skills_mcp_servers(workspace, clients, developer_locs, original) + apply_mcp_server_changes( + original, working, clients, workspace, profile, use_pat=use_pat + ) + state["mcp_servers"] = working + except RuntimeError as exc: + print_warning( + "Could not remove MCP servers/skills previously applied from your workspace's managed " + f"config: {exc}. ucode will retry on the next run." + ) + return False + state.pop("managed_mcp_servers", None) + state.pop("managed_skill_locations", None) save_state(state) - return desired + print_note( + "Removed MCP servers/skills that a previous ucode applied from your workspace's managed " + "config — manage them yourself with `ucode mcp` / `ucode skills`." + ) + return True def _resolve_mcp_selection( @@ -1692,9 +1613,9 @@ def prompt_for_mcp_search_sources(exclude_sources: set[str] | None = None) -> se """First wizard step: choose which sources to search. Returns the set of selected source keys, or `None` if the user cancelled (Ctrl-C). - ``exclude_sources`` drops source keys the caller can't use — e.g. `ucode setup` excludes - ``apps`` because a managed config can't carry an app's off-workspace host, so offering it would - let an admin pick a server that is then silently dropped.""" + ``exclude_sources`` drops source keys the caller can't use — e.g. ``apps``, which a managed + config can't carry (an app's host is off-workspace). No caller sets it today; the param is kept + for that use.""" excluded = exclude_sources or set() choices = [ questionary.Choice(title=label, value=key, checked=checked) @@ -1798,16 +1719,16 @@ def add_mcp_command( """`ucode mcp add`: register Databricks MCP servers WITHOUT removing any that are already configured. - Uses the same discovery and options as `configure mcp` — the interactive + Uses the same discovery and options as `ucode mcp` — the interactive picker, or the non-interactive `--location`/`--services` paths — but is purely - additive: unlike `configure mcp`, it never removes servers outside the + additive: unlike `ucode mcp`, it never removes servers outside the selection. ``agents`` scopes the registration to that subset of configured MCP clients (the agents must already be configured — the `--agents` CLI option sets up any that aren't before calling this).""" if services is not None and not services: - # An empty `--services` selects nothing. For `configure mcp` that means + # An empty `--services` selects nothing. For `ucode mcp` that means # "remove all"; for the additive `add` there is simply nothing to register, # so it's a no-op (and doesn't need --location the way a real subset does). print_note("No MCP services given to add (empty --services); nothing to do.") @@ -1824,8 +1745,8 @@ def configure_mcp_command( agents: set[str] | None = None, ) -> int: """Interactive MCP picker. ``exclude_sources`` hides search sources the caller can't use — - `ucode setup` passes ``{"apps"}`` because a managed config can't carry an app's off-workspace - host, so an app picked here would be silently dropped from the published config. + e.g. ``apps``, which a managed config can't carry (an app's host is off-workspace). No caller + sets it today; the param is kept for that use. ``append`` (used by `ucode mcp add`) makes the command purely additive: the final server list is unioned with the already-configured servers, so nothing @@ -1878,12 +1799,10 @@ def configure_mcp_command( excluded_sources = exclude_sources or set() original_mcp_servers: list[dict] = list(state.get("mcp_servers") or []) - # Skills connections are managed by `configure skills`, so keep them out of + # Skills connections are managed by `ucode 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] - # Drop already-registered servers from an excluded source too (e.g. a previously-added app under - # `ucode setup`), so the picker never shows a server the caller couldn't re-add. if "apps" in excluded_sources: picker_servers = [s for s in picker_servers if not _is_app_mcp_server(s)] original_by_name = _servers_by_name(picker_servers) @@ -1981,7 +1900,7 @@ def configure_mcp_command( def _mcp_change_summary(added: list[str], removed: list[str], clients: list[str]) -> str: - """Human-readable one-liner describing what `configure mcp` just saved, e.g. + """Human-readable one-liner describing what `ucode mcp` just saved, e.g. `Added 2, removed 1 MCP server across Claude Code, Codex`. Falls back to a plain `Saved` when only client bindings changed (no add/remove).""" client_names = ", ".join(str(MCP_CLIENTS[c]["display"]) for c in clients if c in MCP_CLIENTS) @@ -2027,7 +1946,7 @@ def remove_mcp_command(agents: set[str] | None = None) -> int: """`ucode mcp remove`: interactively unregister configured MCP servers. Shows the servers currently configured (skills connections excluded — they're - owned by `configure skills`) and removes the ones you select. It never adds or + owned by `ucode skills`) and removes the ones you select. It never adds or reconfigures anything, and needs no Databricks auth. Without ``agents``, a selected server is removed from every coding tool it's diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 2168b522..e6aa9d9d 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -396,43 +396,6 @@ def download_skills( ) -def download_managed_skills_on_launch( - workspace: str, token: str, locations: list[str], path: str | None = None -) -> list[str]: - """Download admin-published skills to disk so the agent's ``/skills`` lists them. - - Runs on the managed launch path: the config only registers the skills MCP - connection, so nothing else writes the bundles that ``/skills`` reads. Writes - only skills not already on disk -- no overwrite prompt, so the launch never - blocks on input and a developer's own same-named skill is never clobbered. - Best-effort and never raises, so it can't block the launch. Returns the bundle - names newly written. - """ - roots = skill_dir_roots(path) - written: list[str] = [] - for location in locations: - if location.count(".") != 1: - continue - catalog, schema = location.split(".") - refs, reason = list_schema_skills(workspace, token, catalog, schema) - if reason: - print_warning(f"Could not list workspace skills in `{location}`: {reason}.") - continue - refs = _reject_bundle_name_collisions(refs, location=location) - missing = [ref for ref in refs if not existing_skill_on_disk(roots, ref.bundle_name)] - if not missing: - continue - bundles = _fetch_bundles(workspace, token, catalog, schema, missing) - for ref in missing: - files, reason = bundles[ref.securable_name] - if reason or files is None: - print_warning(f"Skipping `{location}.{ref.securable_name}`: {reason}.") - continue - write_skill(roots, ref, files) - written.append(ref.bundle_name) - return written - - def configure_skills_download_command( locations: list[str], *, path: str | None, skills: set[str] | None = None ) -> int: diff --git a/tests/test_cli.py b/tests/test_cli.py index 2a51231e..62f284a6 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -892,8 +892,6 @@ def test_status_shows_managed_config_box_when_present_and_enabled(self, monkeypa monkeypatch.setenv("ENABLE_MANAGED_AGENT_CONFIG", "1") managed = { "enabled_agents": {"claude": {}, "codex": {}}, - "mcp_servers": [{"name": "github-mcp", "type": "external"}], - "skills": {"names": ["debug-ci"]}, } with ( patch("ucode.cli.load_state", return_value=MINIMAL_STATE), @@ -904,8 +902,7 @@ def test_status_shows_managed_config_box_when_present_and_enabled(self, monkeypa assert result.exit_code == 0, result.output assert "Workspace-managed config" in result.output assert "Enabled agents:" in result.output - assert "github-mcp" in result.output - assert "debug-ci" in result.output + assert "Claude Code" in result.output def test_status_hides_managed_config_box_when_feature_disabled(self, monkeypatch): monkeypatch.delenv("ENABLE_MANAGED_AGENT_CONFIG", raising=False) @@ -936,43 +933,37 @@ def test_status_hides_managed_config_box_when_none_present(self, monkeypatch): 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"]) + result = runner.invoke(app, ["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"]) + result = runner.invoke(app, ["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_default_mode_dispatches_download_with_path(self): with patch("ucode.cli.configure_skills_download_command") as mock_download: - result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--path", "/tmp/skills"] - ) + result = runner.invoke(app, ["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", skills=None) def test_default_mode_without_path_dispatches_download(self): with patch("ucode.cli.configure_skills_download_command") as mock_download: - result = runner.invoke(app, ["configure", "skills", "--location", "a.b"]) + result = runner.invoke(app, ["skills", "--location", "a.b"]) assert result.exit_code == 0, result.output mock_download.assert_called_once_with(["a.b"], path=None, skills=None) def test_skill_filter_dispatches_download_with_subset(self): with patch("ucode.cli.configure_skills_download_command") as mock_download: - result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--skill", "my_skill"] - ) + result = runner.invoke(app, ["skills", "--location", "a.b", "--skill", "my_skill"]) assert result.exit_code == 0, result.output mock_download.assert_called_once_with(["a.b"], path=None, skills={"my_skill"}) def test_skill_filter_parses_comma_list(self): with patch("ucode.cli.configure_skills_download_command") as mock_download: - result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--skill", "s1, s2"] - ) + result = runner.invoke(app, ["skills", "--location", "a.b", "--skill", "s1, s2"]) assert result.exit_code == 0, result.output mock_download.assert_called_once_with(["a.b"], path=None, skills={"s1", "s2"}) @@ -982,7 +973,7 @@ def test_skill_with_mcp_exit_1(self): patch("ucode.cli.configure_skills_download_command") as mock_download, ): result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--mcp", "--skill", "my_skill"] + app, ["skills", "--location", "a.b", "--mcp", "--skill", "my_skill"] ) assert result.exit_code == 1 assert "--skill" in _strip_ansi(result.output) @@ -994,7 +985,7 @@ def test_skill_without_location_exit_1(self): patch("ucode.cli.configure_skills_mcp_command") as mock_mcp, patch("ucode.cli.configure_skills_download_command") as mock_download, ): - result = runner.invoke(app, ["configure", "skills", "--skill", "my_skill"]) + result = runner.invoke(app, ["skills", "--skill", "my_skill"]) assert result.exit_code == 1 assert "--skill" in _strip_ansi(result.output) mock_mcp.assert_not_called() @@ -1002,9 +993,7 @@ def test_skill_without_location_exit_1(self): def test_skill_with_multiple_locations_exit_1(self): with patch("ucode.cli.configure_skills_download_command") as mock_download: - result = runner.invoke( - app, ["configure", "skills", "--location", "a.b, c.d", "--skill", "my_skill"] - ) + result = runner.invoke(app, ["skills", "--location", "a.b, c.d", "--skill", "my_skill"]) assert result.exit_code == 1 output = _strip_ansi(result.output) assert "--skill requires a single --location" in output @@ -1016,7 +1005,7 @@ def test_path_with_mcp_exit_1(self): patch("ucode.cli.configure_skills_download_command") as mock_download, ): result = runner.invoke( - app, ["configure", "skills", "--location", "a.b", "--mcp", "--path", "/tmp/skills"] + app, ["skills", "--location", "a.b", "--mcp", "--path", "/tmp/skills"] ) assert result.exit_code == 1 assert "--path" in _strip_ansi(result.output) @@ -1025,26 +1014,26 @@ def test_path_with_mcp_exit_1(self): 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"]) + result = runner.invoke(app, ["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"]) + result = runner.invoke(app, ["skills", "--location", "justone", "--mcp"]) assert result.exit_code == 1 assert "--location" in _strip_ansi(result.output) mock_mcp.assert_not_called() def test_bare_command_registers_schemaless_connection(self): with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: - result = runner.invoke(app, ["configure", "skills"]) + result = runner.invoke(app, ["skills"]) assert result.exit_code == 0, result.output mock_mcp.assert_called_once_with([]) def test_mcp_without_location_registers_schemaless_connection(self): with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: - result = runner.invoke(app, ["configure", "skills", "--mcp"]) + result = runner.invoke(app, ["skills", "--mcp"]) assert result.exit_code == 0, result.output mock_mcp.assert_called_once_with([]) @@ -1053,83 +1042,13 @@ def test_path_without_location_exit_1(self): patch("ucode.cli.configure_skills_mcp_command") as mock_mcp, patch("ucode.cli.configure_skills_download_command") as mock_download, ): - result = runner.invoke(app, ["configure", "skills", "--path", "/tmp/skills"]) + result = runner.invoke(app, ["skills", "--path", "/tmp/skills"]) assert result.exit_code == 1 assert "--path" in _strip_ansi(result.output) mock_mcp.assert_not_called() mock_download.assert_not_called() -class TestApplyManagedSkills: - """The launch path both registers the skills MCP connection and downloads bundles to disk.""" - - def _state(self): - return {"workspace": "https://example.databricks.com", "profile": "prod"} - - def test_downloads_managed_skill_schemas_to_disk(self): - managed = {"skills": {"names": ["main.default", "ml.prod"]}} - with ( - patch("ucode.cli.apply_managed_skills", return_value=["main.default"]) as mock_apply, - patch("ucode.cli.get_databricks_token", return_value="tok") as mock_token, - patch( - "ucode.cli.download_managed_skills_on_launch", return_value=["triage"] - ) as mock_dl, - ): - from ucode import cli - - cli._apply_managed_skills(managed, "claude", self._state()) - - mock_apply.assert_called_once() - mock_token.assert_called_once_with("https://example.databricks.com", "prod") - mock_dl.assert_called_once_with( - "https://example.databricks.com", "tok", ["main.default", "ml.prod"] - ) - - def test_no_managed_skills_skips_the_download(self): - with ( - patch("ucode.cli.apply_managed_skills", return_value=[]), - patch("ucode.cli.get_databricks_token") as mock_token, - patch("ucode.cli.download_managed_skills_on_launch") as mock_dl, - ): - from ucode import cli - - cli._apply_managed_skills({}, "claude", self._state()) - - mock_token.assert_not_called() - mock_dl.assert_not_called() - - def test_download_still_runs_when_mcp_registration_fails(self): - # A failure registering the MCP connection must not stop the disk download — the two are - # independent ways skills reach the agent, and /skills depends only on the disk write. - with ( - patch("ucode.cli.apply_managed_skills", side_effect=RuntimeError("boom")), - patch("ucode.cli.get_databricks_token", return_value="tok"), - patch("ucode.cli.download_managed_skills_on_launch", return_value=[]) as mock_dl, - ): - from ucode import cli - - cli._apply_managed_skills( - {"skills": {"names": ["main.default"]}}, "claude", self._state() - ) - - mock_dl.assert_called_once() - - def test_download_failure_never_blocks_launch(self): - with ( - patch("ucode.cli.apply_managed_skills", return_value=[]), - patch("ucode.cli.get_databricks_token", side_effect=RuntimeError("no auth")), - patch("ucode.cli.download_managed_skills_on_launch") as mock_dl, - ): - from ucode import cli - - # Must not raise. - cli._apply_managed_skills( - {"skills": {"names": ["main.default"]}}, "claude", self._state() - ) - - mock_dl.assert_not_called() - - class TestStatusSkillsSection: def _run(self, state): with patch("ucode.cli.load_state", return_value=state): @@ -1924,68 +1843,6 @@ def test_workspaces_flag_rejects_empty_list(self): mock_cfg.assert_not_called() -class TestConfigureMcpFlag: - def test_mcp_with_agents_configures_then_registers_services(self): - with ( - patch("ucode.cli.install_databricks_cli"), - patch("ucode.cli.install_tool_binary"), - patch("ucode.cli.configure_workspace_command") as mock_cfg, - patch("ucode.cli.configure_mcp_command") as mock_mcp, - ): - result = runner.invoke( - app, - ["configure", "--agents", "claude", "--mcp", "system.ai.slack,system.ai.github"], - ) - assert result.exit_code == 0, result.output - mock_cfg.assert_called_once_with( - selected_tools=["claude"], - prompt_optional_updates=True, - ) - mock_mcp.assert_called_once_with(services={"system.ai.slack", "system.ai.github"}) - - def test_mcp_only_configures_workspace_without_agent_picker(self): - # `--mcp` with no --agents (e.g. Cursor): configure the workspace directly, - # never the interactive agent picker, then register the MCP service. - with ( - patch("ucode.cli.install_databricks_cli"), - patch("ucode.cli.configure_workspace_command") as mock_cfg, - patch("ucode.cli._configure_shared_workspace_states") as mock_shared, - patch("ucode.cli.configure_mcp_command") as mock_mcp, - ): - result = runner.invoke( - app, - [ - "configure", - "--workspaces", - "https://ws.databricks.com", - "--mcp", - "system.ai.slack", - ], - ) - assert result.exit_code == 0, result.output - # Never the model-agent picker path. - mock_cfg.assert_not_called() - mock_shared.assert_called_once() - # Workspace-only: no model tools fetched. - assert ( - mock_shared.call_args.kwargs.get("tools") == [] or mock_shared.call_args.args[1] == [] - ) - mock_mcp.assert_called_once_with(services={"system.ai.slack"}) - - def test_mcp_rejects_bare_short_name(self): - with ( - patch("ucode.cli.install_databricks_cli"), - patch("ucode.cli.configure_workspace_command"), - patch("ucode.cli._configure_shared_workspace_states"), - patch("ucode.cli.configure_mcp_command") as mock_mcp, - ): - result = runner.invoke( - app, ["configure", "--workspaces", "https://ws.databricks.com", "--mcp", "slack"] - ) - assert result.exit_code != 0 - mock_mcp.assert_not_called() - - class TestConfigureAgentsSelection: def test_selected_tools_skip_picker(self, monkeypatch): import ucode.cli as cli_mod @@ -3541,31 +3398,6 @@ def test_lists_the_tiers_and_the_applied_model(self, capsys): assert "at 80%" in out and "OpenCode" in out and "haiku" in out assert "system.ai.opus" in out - def test_lists_managed_mcps_and_skills(self, capsys): - import ucode.cli as cli_mod - - managed = { - **self.MANAGED, - "mcp_servers": [{"name": "system.ai.slack", "type": "mcp-service"}], - "skills": {"names": ["main.default.my_skill"]}, - } - cli_mod._print_managed_summary(managed, {"workspace": "https://w"}, "claude") - out = capsys.readouterr().out - assert "system.ai.slack" in out - assert "main.default.my_skill" in out - # Marked pending until ucode registers them locally. - assert "pending" in out - - def test_mcp_and_skill_rows_say_none_when_the_config_names_none(self, capsys): - import ucode.cli as cli_mod - - # Shown rather than omitted: a missing row leaves "my admin set none" ambiguous. - cli_mod._print_managed_summary(self.MANAGED, {"workspace": "https://w"}, "claude") - out = capsys.readouterr().out - assert "MCPs:" in out and "Skills:" in out - assert out.count("none configured") == 2 - assert "pending" not in out - def test_no_policy_rows_without_a_budget_policy(self, capsys): import ucode.cli as cli_mod @@ -3630,8 +3462,6 @@ def test_launch_banner_is_abridged_not_the_full_box(self, monkeypatch): managed = { "default_agent": "claude", "enabled_agents": {"claude": {"model_config": {"default_model": "system.ai.opus"}}}, - "mcp_servers": [{"name": "system.ai.slack", "type": "mcp-service"}], - "skills": {"names": ["main.default.my_skill"]}, } result, _ = self._run(monkeypatch, managed=managed) assert result.exit_code == 0, result.output @@ -3640,8 +3470,6 @@ def test_launch_banner_is_abridged_not_the_full_box(self, monkeypatch): assert "system.ai.opus" in result.output # The full box's per-config enumeration is left to `ucode status`. assert "Enabled agents:" not in result.output - assert "system.ai.slack" not in result.output - assert "main.default.my_skill" not in result.output def test_launch_banner_omits_default_agent_when_a_tier_overrides(self, monkeypatch): # A budget tier can launch a different agent than the config's default; the banner must not diff --git a/tests/test_databricks.py b/tests/test_databricks.py index 836e9f8f..d1420571 100644 --- a/tests/test_databricks.py +++ b/tests/test_databricks.py @@ -3124,7 +3124,26 @@ def test_update_mask_covers_every_field_the_manifest_can_set(self): } ) ) - assert set(db_mod.MANAGED_CONFIG_UPDATE_MASK_PATHS) == emitted | {"spec_version"} + mask = set(db_mod.MANAGED_CONFIG_UPDATE_MASK_PATHS) + assert emitted | {"spec_version"} <= mask + assert mask - emitted - {"spec_version"} == {"mcp_servers", "skills"} + + def test_update_mask_clears_legacy_mcp_and_skills(self): + from ucode.managed_setup import serialize_managed_config + + payload = serialize_managed_config( + { + "default_agent": "claude", + "enabled_agents": { + "claude": {"model_config": {"default_model": "system.ai.claude-opus-5"}} + }, + "mcp_servers": [{"name": "databricks-sql", "type": "sql"}], + "skills": {"names": ["main.default"]}, + } + ) + assert "mcp_servers" not in payload + assert "skills" not in payload + assert {"mcp_servers", "skills"} <= set(db_mod.MANAGED_CONFIG_UPDATE_MASK_PATHS) def test_delete_returns_only_a_reason(self, monkeypatch): seen = {} diff --git a/tests/test_managed_config.py b/tests/test_managed_config.py index 05c179fc..3ecf1d07 100644 --- a/tests/test_managed_config.py +++ b/tests/test_managed_config.py @@ -102,16 +102,13 @@ def test_opencode_model_list_is_flat(self): "system.ai.kimi-k2-7-code", ] - def test_mcp_servers_map_type_enums_to_tags(self): - mcp = normalize_managed_config(RAW_MANIFEST)["mcp_servers"] - assert mcp == [ - {"name": "system.ai.github", "type": "mcp-service"}, - {"name": "some-space-id", "type": "genie-space"}, - ] + def test_mcp_servers_and_skills_are_dropped_on_read(self): + cfg = normalize_managed_config(RAW_MANIFEST) + assert "mcp_servers" not in cfg + assert "skills" not in cfg - def test_skills_and_tracing_and_budget(self): + def test_tracing_and_budget(self): cfg = normalize_managed_config(RAW_MANIFEST) - assert cfg["skills"] == {"names": ["system.ai.pdf-extraction"]} assert cfg["tracing_table"] == "main.default.ucode_traces" assert cfg["budget_policy"]["budget_id"] == "c6563b45-df9a-4b19-afb2-d42dc2b52576" assert cfg["budget_policy"]["tiers"][1]["default_agent"] == "opencode" @@ -130,10 +127,6 @@ def test_unrecognized_agent_enum_dropped(self, agent_enum): raw = {"enabled_agents": [{"agent": agent_enum, "config": {}}]} assert "enabled_agents" not in normalize_managed_config(raw) - def test_unknown_mcp_type_dropped(self): - raw = {"mcp_servers": [{"name": "x", "type": "MCP_SERVER_TYPE_UNSPECIFIED"}]} - assert "mcp_servers" not in normalize_managed_config(raw) - def test_empty_manifest_yields_empty_dict(self): assert normalize_managed_config({}) == {} diff --git a/tests/test_managed_export.py b/tests/test_managed_export.py index b11b3b07..a066b84d 100644 --- a/tests/test_managed_export.py +++ b/tests/test_managed_export.py @@ -37,8 +37,6 @@ "claude": {"model_config": {"default_model": "system.ai.claude-opus-4-8"}}, "codex": {"model_config": {"default_model": "system.ai.gpt-5-6"}}, }, - "mcp_servers": [{"name": "system.ai.slack", "type": "mcp-service"}], - "skills": {"names": ["main.default"]}, } @@ -66,9 +64,6 @@ def test_excludes_server_owned_resource_name(self): payload = export_mod.build_export_payload() assert "name" not in payload assert payload["default_agent"] == "CODING_AGENT_CLAUDE_CODE" - assert payload["mcp_servers"] == [ - {"name": "system.ai.slack", "type": "MCP_SERVER_TYPE_UC_SERVICE"} - ] def test_envelope_workspace_first_then_spec_version(self): with _with_manifest(FULL_MANIFEST): diff --git a/tests/test_managed_publish.py b/tests/test_managed_publish.py index 37276b60..83abc8eb 100644 --- a/tests/test_managed_publish.py +++ b/tests/test_managed_publish.py @@ -24,8 +24,6 @@ "claude": {"model_config": {"default_model": "system.ai.claude-opus-4-8"}}, "codex": {"model_config": {"default_model": "system.ai.gpt-5-6"}}, }, - "mcp_servers": [{"name": "system.ai.slack", "type": "mcp-service"}], - "skills": {"names": ["main.default"]}, } @@ -138,6 +136,16 @@ def test_unknown_top_level_field_is_rejected(self): with pytest.raises(RuntimeError, match="does not recognize"): parse_publish_payload(_payload(bogus="value"), WORKSPACE) + def test_legacy_mcp_and_skills_are_ignored_not_rejected(self): + payload = _payload( + mcp_servers=[{"name": "system.ai.slack", "type": "MCP_SERVER_TYPE_UC_SERVICE"}], + skills={"names": ["main.default"]}, + ) + manifest, api_payload = parse_publish_payload(payload, WORKSPACE) + assert "mcp_servers" not in manifest and "skills" not in manifest + assert "mcp_servers" not in api_payload and "skills" not in api_payload + assert manifest["default_agent"] == "claude" + def test_nested_unknown_field_is_rejected(self): payload = _payload() payload["enabled_agents"][0]["config"] = {"unknown_setting": True} diff --git a/tests/test_managed_setup.py b/tests/test_managed_setup.py index 140bc3d9..90513bac 100644 --- a/tests/test_managed_setup.py +++ b/tests/test_managed_setup.py @@ -11,12 +11,10 @@ from ucode.managed_config import ( AGENT_ENUM_TO_TOOL, - MCP_TYPE_ENUM_TO_TAG, normalize_managed_config, ) from ucode.managed_setup import ( AGENT_TOOL_TO_ENUM, - MCP_TAG_TO_TYPE_ENUM, claude_family_for_model, claude_model_slots, model_families_for_agent, @@ -84,11 +82,6 @@ def _full_manifest() -> dict: }, }, }, - "mcp_servers": [ - {"name": "system.ai.github", "type": "mcp-service"}, - {"name": "genie-space-id", "type": "genie-space"}, - ], - "skills": {"names": ["system.ai.pdf-extraction"]}, "tracing_table": "main.default.ucode-traces", "budget_policy": { "display_name": "eng-tiered-routing", @@ -113,9 +106,6 @@ class TestEnumMaps: def test_agent_map_is_the_inverse_of_the_read_side(self): assert AGENT_TOOL_TO_ENUM == {tool: enum for enum, tool in AGENT_ENUM_TO_TOOL.items()} - def test_mcp_map_is_the_inverse_of_the_read_side(self): - assert MCP_TAG_TO_TYPE_ENUM == {tag: enum for enum, tag in MCP_TYPE_ENUM_TO_TAG.items()} - def test_agent_map_round_trips(self): for tool, enum in AGENT_TOOL_TO_ENUM.items(): assert AGENT_ENUM_TO_TOOL[enum] == tool @@ -123,7 +113,6 @@ def test_agent_map_round_trips(self): def test_inversion_is_lossless(self): # A duplicated tool name on the read side would silently collapse an entry here. assert len(AGENT_TOOL_TO_ENUM) == len(AGENT_ENUM_TO_TOOL) - assert len(MCP_TAG_TO_TYPE_ENUM) == len(MCP_TYPE_ENUM_TO_TAG) class TestRoundTrip: @@ -152,11 +141,6 @@ def test_every_known_agent_round_trips(self): } assert normalize_managed_config(serialize_managed_config(manifest)) == manifest, tool - def test_every_mcp_type_round_trips(self): - for tag in MCP_TAG_TO_TYPE_ENUM: - manifest = {"mcp_servers": [{"name": "some-server", "type": tag}]} - assert normalize_managed_config(serialize_managed_config(manifest)) == manifest, tag - class TestSerialize: def test_maps_tool_names_to_proto_enums(self): @@ -223,12 +207,16 @@ def test_model_provider_service_is_carried_through(self): variant = payload["enabled_agents"][0]["config"]["model_config"]["claude"] assert variant["model_provider_service"] == "main.default.anthropic-mps" - def test_mcp_types_map_to_proto_enums(self): - payload = serialize_managed_config(_full_manifest()) - assert payload["mcp_servers"] == [ - {"name": "system.ai.github", "type": "MCP_SERVER_TYPE_UC_SERVICE"}, - {"name": "genie-space-id", "type": "MCP_SERVER_TYPE_GENIE"}, - ] + def test_mcp_servers_and_skills_are_never_serialized(self): + payload = serialize_managed_config( + { + **_full_manifest(), + "mcp_servers": [{"name": "system.ai.github", "type": "mcp-service"}], + "skills": {"names": ["system.ai.pdf-extraction"]}, + } + ) + assert "mcp_servers" not in payload + assert "skills" not in payload def test_tracing_becomes_a_table_object(self): payload = serialize_managed_config(_full_manifest()) @@ -277,12 +265,6 @@ def test_unknown_agent_is_dropped(self): "CODING_AGENT_CLAUDE_CODE" ] - def test_unknown_mcp_type_is_dropped(self): - payload = serialize_managed_config( - {"mcp_servers": [{"name": "a", "type": "not-a-type"}, {"name": "b", "type": "sql"}]} - ) - assert payload["mcp_servers"] == [{"name": "b", "type": "MCP_SERVER_TYPE_DATABRICKS_SQL"}] - def test_empty_manifest_serializes_to_empty_payload(self): assert serialize_managed_config({}) == {} @@ -614,17 +596,12 @@ def test_model_check_skipped_for_provider_service(self): } assert validate_manifest(manifest, STATE) == [] - def test_mcp_server_needs_a_name(self): - errors = validate_manifest({"mcp_servers": [{"type": "sql"}]}) - assert any("name is required" in e for e in errors) - - def test_mcp_server_needs_a_known_type(self): - errors = validate_manifest({"mcp_servers": [{"name": "a", "type": "bogus"}]}) - assert any("is not recognized" in e for e in errors) - - def test_empty_skill_name_is_rejected(self): - errors = validate_manifest({"skills": {"names": ["ok", ""]}}) - assert any("skills.names" in e for e in errors) + def test_mcp_servers_and_skills_are_not_validated(self): + errors = validate_manifest( + {**_minimal_manifest(), "mcp_servers": [{"type": "bogus"}], "skills": {"names": [""]}}, + STATE, + ) + assert errors == [] def test_empty_tracing_table_is_rejected(self): errors = validate_manifest({"tracing_table": ""}) @@ -904,7 +881,18 @@ def test_tier_positions_are_reported_zero_based(self): def test_errors_accumulate(self): manifest = { "default_agent": "codex", - "enabled_agents": {"claude": {}}, - "mcp_servers": [{"type": "bogus"}], + "enabled_agents": { + "claude": {"model_config": {"default_model": "system.ai.claude-opus-4-8"}}, + "not-an-agent": {}, + }, + "budget_policy": { + "tiers": [ + { + "spending_percentage": 0.5, + "default_agent": "claude", + "default_model": "system.ai.claude-opus-4-8", + } + ] + }, } assert len(validate_manifest(manifest, STATE)) >= 3 diff --git a/tests/test_managed_wizard.py b/tests/test_managed_wizard.py index 0fcbebac..90060a35 100644 --- a/tests/test_managed_wizard.py +++ b/tests/test_managed_wizard.py @@ -71,129 +71,6 @@ def test_malformed_tracing_yields_none(self): assert wizard._tracing_table_from_state({"tracing": "on"}) is None -class TestMcpServerFromUrl: - @pytest.mark.parametrize( - ("url", "expected"), - [ - # mcp-service stores the dash form the launch path rebuilds the dotted URL from. - ( - "https://ws.example.com/ai-gateway/mcp-services/system.ai.github", - ("system-ai-github", "mcp-service"), - ), - ("https://ws.example.com/api/2.0/mcp/external/jira-prod", ("jira-prod", "external")), - ("https://ws.example.com/api/2.0/mcp/genie/01ef", ("01ef", "genie-space")), - # vector-search / uc-functions store `.`, not the local display slug. - ( - "https://ws.example.com/api/2.0/mcp/vector-search/my_cat/my_schema", - ("my_cat.my_schema", "vector-search"), - ), - ( - "https://ws.example.com/api/2.0/mcp/functions/dev_cat/dev_fixture", - ("dev_cat.dev_fixture", "uc-functions"), - ), - ("https://ws.example.com/api/2.0/mcp/sql", ("databricks-sql", "sql")), - ], - ) - def test_known_urls(self, url, expected): - assert wizard._mcp_server_from_url(url) == expected - - def test_apps_are_not_publishable(self): - # An app's host isn't reconstructable from the workspace + an id, so it can't be published. - assert ( - wizard._mcp_server_from_url("https://mcp-myapp-123.aws.databricksapps.com/mcp") is None - ) - - def test_unknown_url_yields_none(self): - assert wizard._mcp_server_from_url("https://example.com/something/else") is None - - def test_vector_search_needs_both_catalog_and_schema(self): - assert ( - wizard._mcp_server_from_url("https://ws.example.com/api/2.0/mcp/functions/onlycat") - is None - ) - - -class TestMcpServersFromState: - def test_maps_registered_servers_to_name_and_type(self): - state = { - "mcp_servers": [ - { - "name": "databricks-github", - "url": f"{WORKSPACE}/ai-gateway/mcp-services/system.ai.github", - }, - {"name": "databricks-sql", "url": f"{WORKSPACE}/api/2.0/mcp/sql"}, - ] - } - # The published name comes from the URL (the identifier the server field holds), not the - # local display name. - assert wizard._mcp_servers_from_state(state) == [ - {"name": "system-ai-github", "type": "mcp-service"}, - {"name": "databricks-sql", "type": "sql"}, - ] - - def test_publishes_catalog_schema_for_uc_functions(self): - # The lossy local slug is replaced with the dotted catalog.schema the launch path can split. - state = { - "mcp_servers": [ - { - "name": "databricks-functions-dev-cat-dev-fixture", - "url": f"{WORKSPACE}/api/2.0/mcp/functions/dev_cat/dev_fixture", - }, - ] - } - assert wizard._mcp_servers_from_state(state) == [ - {"name": "dev_cat.dev_fixture", "type": "uc-functions"}, - ] - - def test_skips_the_skills_registry_entry(self): - # Skills are published under the manifest's own `skills` field; including the MCP entry too - # would configure them twice. - from ucode.mcp import SKILLS_MCP_KIND - - state = { - "mcp_servers": [ - { - "name": "databricks-skill-registry", - "kind": SKILLS_MCP_KIND, - "url": f"{WORKSPACE}/api/2.0/mcp/sql", - }, - {"name": "databricks-sql", "url": f"{WORKSPACE}/api/2.0/mcp/sql"}, - ] - } - assert wizard._mcp_servers_from_state(state) == [{"name": "databricks-sql", "type": "sql"}] - - def test_skips_apps_and_unclassifiable_servers(self): - state = { - "mcp_servers": [ - {"name": "mystery", "url": "https://example.com/nope"}, - {"name": "databricks-app-x", "url": "https://x-1.databricksapps.com/mcp"}, - ] - } - assert wizard._mcp_servers_from_state(state) == [] - - def test_skips_entries_missing_name_or_url(self): - state = { - "mcp_servers": [ - {"url": f"{WORKSPACE}/api/2.0/mcp/sql"}, - {"name": "no-url"}, - "not-a-dict", - ] - } - assert wizard._mcp_servers_from_state(state) == [] - - def test_empty_state_yields_nothing(self): - assert wizard._mcp_servers_from_state({}) == [] - - def test_output_validates_as_a_manifest(self): - state = { - "mcp_servers": [ - {"name": "databricks-sql", "url": f"{WORKSPACE}/api/2.0/mcp/sql"}, - ] - } - servers = wizard._mcp_servers_from_state(state) - assert validate_manifest({"mcp_servers": servers}) == [] - - class TestAdminGate: def test_non_admin_is_rejected(self): with patch.object(wizard, "is_workspace_admin", return_value=False): @@ -2143,8 +2020,6 @@ class TestCarryForwardSections: def test_all_optional_sections_survive_a_rerun(self): previous = { **AGENTS_ONLY, - "mcp_servers": [{"name": "system.ai.github", "type": "mcp-service"}], - "skills": {"names": ["main.default"]}, "tracing_table": "main.default.traces", "budget_policy": { "budget_id": BUDGET_ID, @@ -2159,11 +2034,20 @@ def test_all_optional_sections_survive_a_rerun(self): } manifest = dict(AGENTS_ONLY) wizard._carry_forward_sections(previous, manifest) - assert manifest["mcp_servers"] == previous["mcp_servers"] - assert manifest["skills"] == previous["skills"] assert manifest["tracing_table"] == previous["tracing_table"] assert manifest["budget_policy"] == previous["budget_policy"] + def test_mcp_and_skills_are_not_carried(self): + previous = { + **AGENTS_ONLY, + "mcp_servers": [{"name": "system.ai.github", "type": "mcp-service"}], + "skills": {"names": ["main.default"]}, + } + manifest = dict(AGENTS_ONLY) + wizard._carry_forward_sections(previous, manifest) + assert "mcp_servers" not in manifest + assert "skills" not in manifest + def test_empty_previous_adds_nothing(self): manifest = dict(AGENTS_ONLY) wizard._carry_forward_sections({}, manifest) @@ -2196,14 +2080,17 @@ def test_budget_policy_naming_a_dropped_agent_is_left_out_with_a_warning(self): class TestNextSteps: def test_marks_configured_and_unconfigured_sections(self, capsys): - manifest = {**AGENTS_ONLY, "skills": {"names": ["main.default"]}} - wizard._print_next_steps(manifest) + wizard._print_next_steps(dict(AGENTS_ONLY)) out = capsys.readouterr().out - assert "ucode setup mcps" in out - assert "ucode setup skills" in out assert "ucode setup spend-tiers" in out + assert "not configured" in out assert "ucode publish" in out + wizard._print_next_steps({**AGENTS_ONLY, "budget_policy": {"budget_id": BUDGET_ID}}) + out = capsys.readouterr().out + assert "ucode setup spend-tiers" in out + assert "not configured" not in out + def test_dry_run_says_nothing_was_saved(self, capsys, monkeypatch): monkeypatch.setattr(config_io_mod, "_dry_run", True) wizard._print_next_steps(AGENTS_ONLY) @@ -2213,7 +2100,7 @@ def test_dry_run_says_nothing_was_saved(self, capsys, monkeypatch): class TestSectionCommands: - """The `ucode setup mcps` / `skills` / `spend-tiers` section commands.""" + """`ucode setup spend-tiers` — the one managed-config section command, strictly admin-only.""" @staticmethod def _admin(**overrides): @@ -2240,96 +2127,25 @@ def _run(self, fn, *, admin_overrides=None, **patches): stack.enter_context(patch.object(wizard, name, value)) return fn() - def test_mcp_requires_an_authored_config(self): + def test_requires_an_authored_config(self): # No manifest on disk → the command can't edit a section that doesn't exist. with pytest.raises(RuntimeError, match="ucode setup"): - self._run(wizard.setup_mcp_command) + self._run(wizard.setup_budget_policy_command) - def test_mcp_requires_enabled_agents(self): + def test_requires_enabled_agents(self): # A launch stores `{}` to mean "no managed config"; that must not count as authored. managed_config_mod.save_managed_state(WORKSPACE, {}) with pytest.raises(RuntimeError, match="ucode setup"): - self._run(wizard.setup_mcp_command) - - def test_mcp_writes_only_its_section(self): - managed_config_mod.save_managed_state(WORKSPACE, AGENTS_ONLY) - servers = [{"name": "system.ai.github", "type": "mcp-service"}] - # The picker (imported lazily inside the command) registers servers into local state; fake - # that by having the before/after reads bracket a change. - reads = iter([[], servers]) - with ( - patch("ucode.mcp.configure_mcp_command", return_value=0) as picker, - patch.object(wizard, "_mcp_servers_from_state", side_effect=lambda *_: next(reads)), - ): - code = self._run(wizard.setup_mcp_command) - assert code == 0 - assert picker.call_args.kwargs == {"exclude_sources": {"apps"}} - saved = managed_config_mod.load_managed_state(WORKSPACE) - assert saved["mcp_servers"] == servers - assert saved["enabled_agents"] == AGENTS_ONLY["enabled_agents"] - - def test_mcp_cancel_is_a_no_op(self): - # Picker cancelled / nothing changed → the section is left exactly as it was. - managed_config_mod.save_managed_state(WORKSPACE, AGENTS_ONLY) - with ( - patch("ucode.mcp.configure_mcp_command", return_value=0), - patch.object(wizard, "_mcp_servers_from_state", return_value=[]), - patch.object(wizard, "save_managed_state") as save, - ): - code = self._run(wizard.setup_mcp_command) - assert code == 0 - assert not save.called - - def test_mcp_carries_forward_preregistered_servers(self): - # An admin who ran `ucode configure mcp` first arrives with those servers already registered, - # so the picker leaves local state unchanged (before == after). The manifest doesn't carry them - # yet, so `setup mcps` must still save them rather than report "no changes" and drop them. - managed_config_mod.save_managed_state(WORKSPACE, AGENTS_ONLY) - servers = [{"name": "system.ai.github", "type": "mcp-service"}] - with ( - patch("ucode.mcp.configure_mcp_command", return_value=0), - patch.object(wizard, "_mcp_servers_from_state", return_value=servers), - ): - code = self._run(wizard.setup_mcp_command) - assert code == 0 - assert managed_config_mod.load_managed_state(WORKSPACE)["mcp_servers"] == servers + self._run(wizard.setup_budget_policy_command) - def test_mcp_not_admin_raises(self): + def test_not_admin_raises(self): managed_config_mod.save_managed_state(WORKSPACE, AGENTS_ONLY) with pytest.raises(RuntimeError, match="not an admin"): self._run( - wizard.setup_mcp_command, + wizard.setup_budget_policy_command, admin_overrides={"is_workspace_admin": lambda *a, **k: False}, ) - def test_skills_location_bypasses_the_prompt(self): - managed_config_mod.save_managed_state(WORKSPACE, AGENTS_ONLY) - with ( - patch("ucode.mcp.configure_skills_mcp_command", return_value=0) as configure, - patch.object(wizard, "_skill_names_from_state", return_value=["main.default"]), - patch.object(wizard, "prompt_for_text") as prompt, - ): - code = self._run(lambda: wizard.setup_skills_command(["main.default"])) - assert code == 0 - assert not prompt.called - configure.assert_called_once_with(["main.default"]) - assert managed_config_mod.load_managed_state(WORKSPACE)["skills"] == { - "names": ["main.default"] - } - - def test_skills_blank_answer_writes_nothing(self): - # A blank answer must not delegate: `configure_skills_mcp_command([])` is not a no-op. - managed_config_mod.save_managed_state(WORKSPACE, AGENTS_ONLY) - with ( - patch("ucode.mcp.configure_skills_mcp_command") as configure, - patch.object(wizard, "prompt_for_text", return_value=""), - patch.object(wizard, "save_managed_state") as save, - ): - code = self._run(wizard.setup_skills_command) - assert code == 0 - assert not configure.called - assert not save.called - def test_budget_policy_offers_only_the_manifests_agents(self): managed_config_mod.save_managed_state(WORKSPACE, AGENTS_ONLY) captured = {} @@ -2374,8 +2190,6 @@ def test_lists_every_setup_command(self, capsys): out = capsys.readouterr().out for command in ( "ucode setup", - "ucode setup mcps", - "ucode setup skills", "ucode setup spend-tiers", "ucode setup show", "ucode publish", @@ -2387,22 +2201,25 @@ class TestPublishDiff: def test_lists_added_removed_and_changed(self, capsys): existing = { "name": "cfg/1", - **AGENTS_ONLY, - "mcp_servers": [{"name": "system.ai.slack", "type": "mcp-service"}], + "default_agent": "claude", + "enabled_agents": { + "claude": {"model_config": {"default_model": "system.ai.claude-opus-4-8"}}, + "codex": {"model_config": {"default_model": "system.ai.gpt-5-6"}}, + }, } incoming = { "default_agent": "claude", "enabled_agents": { "claude": {"model_config": {"default_model": "system.ai.claude-opus-4-9"}} }, - "mcp_servers": [{"name": "system.ai.github", "type": "mcp-service"}], + "tracing_table": "main.default.traces", } changed = wizard._render_config_diff(existing, incoming, WORKSPACE) out = capsys.readouterr().out assert changed is True assert "CHANGE" in out and "claude-opus-4-8" in out and "claude-opus-4-9" in out - assert "ADD" in out and "system.ai.github" in out # added server - assert "DELETE" in out and "system.ai.slack" in out # removed server + assert "ADD" in out and "main.default.traces" in out + assert "DELETE" in out and "system.ai.gpt-5-6" in out def test_identical_configs_report_no_change(self, capsys): assert wizard._render_config_diff(AGENTS_ONLY, AGENTS_ONLY, WORKSPACE) is False @@ -2929,11 +2746,7 @@ def test_show_exits_zero(self): @pytest.mark.parametrize( ("command", "target"), - [ - ("mcps", "setup_mcp_command"), - ("skills", "setup_skills_command"), - ("spend-tiers", "setup_budget_policy_command"), - ], + [("spend-tiers", "setup_budget_policy_command")], ) def test_section_subcommands_are_registered_and_called(self, command, target): with ( @@ -2945,19 +2758,13 @@ def test_section_subcommands_are_registered_and_called(self, command, target): assert fn.called assert "ERROR" not in _out(result) - def test_setup_skills_declares_location(self): - group = typer.main.get_command(app).commands["setup"] # type: ignore[attr-defined] - skills = group.commands["skills"] # type: ignore[attr-defined] - declared = {opt for param in skills.params for opt in param.opts} - assert "--location" in declared + def test_skills_is_a_top_level_command(self): + commands = typer.main.get_command(app).commands # type: ignore[attr-defined] + assert "skills" in commands - def test_setup_skills_location_is_parsed_to_a_list(self): - with ( - patch("ucode.cli.install_databricks_cli"), - patch("ucode.cli.setup_skills_command", return_value=0) as fn, - ): - runner.invoke(app, ["setup", "skills", "--location", "main.a,main.b"]) - assert fn.call_args.args[0] == ["main.a", "main.b"] + def test_mcp_is_a_top_level_group(self): + group = typer.main.get_command(app).commands["mcp"] # type: ignore[attr-defined] + assert {"add", "remove", "web-search"} <= set(group.commands) # type: ignore[attr-defined] def test_setup_help_needs_no_auth(self): # `ucode setup help` reads the local draft only — it must not shell out to install the CLI. @@ -2974,10 +2781,11 @@ def test_section_command_runtime_error_exits_1(self): with ( patch("ucode.cli.install_databricks_cli"), patch( - "ucode.cli.setup_mcp_command", side_effect=RuntimeError("run `ucode setup` first") + "ucode.cli.setup_budget_policy_command", + side_effect=RuntimeError("run `ucode setup` first"), ), ): - result = runner.invoke(app, ["setup", "mcps"]) + result = runner.invoke(app, ["setup", "spend-tiers"]) assert result.exit_code == 1 assert "ucode setup" in _out(result) diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 69c60d0a..f03b74bf 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -1324,7 +1324,7 @@ def test_removes_orphan_mcp_entries_from_other_workspace_buckets(self, monkeypat assert cleanup_calls == [("claude", "orphan-mcp")] def test_skips_orphan_warning_when_nothing_was_actually_removed(self, monkeypatch, capsys): - """Re-running configure mcp on the same workspace shouldn't repeat the warning + """Re-running `ucode mcp` on the same workspace shouldn't repeat the warning if the leftover entries were already removed by a previous run.""" cleanup_calls: list[tuple[str, str]] = [] other_ws = "https://other-workspace.cloud.databricks.com" @@ -1773,7 +1773,7 @@ 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 + """A skills connection is owned by `ucode skills`, so `ucode mcp --location` must leave it registered rather than treating it as a removal.""" saved_states: list[dict] = [] removed: list[tuple[str, str]] = [] @@ -1856,7 +1856,7 @@ class TestAddMcpCommand: """`ucode mcp add` (append) registers new servers without removing existing ones.""" def test_keeps_servers_outside_location(self, monkeypatch): - """Unlike `configure mcp --location`, `mcp add --location` preserves any + """Unlike `ucode mcp --location`, `mcp add --location` preserves any server outside the location instead of removing it.""" saved_states: list[dict] = [] configured: list[tuple[str, str, str]] = [] @@ -2053,7 +2053,7 @@ def fake_prompt(servers): monkeypatch.setattr(mcp, "_prompt_for_mcp_removal", fake_prompt) assert mcp.remove_mcp_command() == 0 - # The skills connection is owned by `configure skills`, so it's never a + # The skills connection is owned by `ucode skills`, so it's never a # removal candidate; only the real MCP server is offered. assert offered["names"] == ["system-ai-github"] @@ -2652,214 +2652,100 @@ def test_drops_foreign_workspace_skills_entry(self, monkeypatch): assert state["mcp_servers"] == [] -class TestManagedMcpServerEntry: - def test_sql(self): - assert mcp.managed_mcp_server_entry("databricks-sql", "sql", WS) == ( - "databricks-sql", - f"{WS}/api/2.0/mcp/sql", - ) - - def test_external_uses_the_connection_name(self): - assert mcp.managed_mcp_server_entry("jira-prod", "external", WS) == ( - "jira-prod", - f"{WS}/api/2.0/mcp/external/jira-prod", - ) - - def test_mcp_service_undashes_catalog_and_schema_only(self): - # The manifest stores the dash form; only the first two dashes (catalog.schema) become dots, - # so a service name keeps its own dashes/underscores. The entry name stays the dash form. - assert mcp.managed_mcp_server_entry("system-ai-dbsql", "mcp-service", WS) == ( - "system-ai-dbsql", - f"{WS}/ai-gateway/mcp-services/system.ai.dbsql", - ) - assert mcp.managed_mcp_server_entry("system-ai-google_calendar", "mcp-service", WS) == ( - "system-ai-google_calendar", - f"{WS}/ai-gateway/mcp-services/system.ai.google_calendar", - ) - - def test_mcp_service_needs_three_parts(self): - assert mcp.managed_mcp_server_entry("justtwo-parts", "mcp-service", WS) is None - - def test_genie_space_uses_the_space_id(self): - assert mcp.managed_mcp_server_entry("01ef9a", "genie-space", WS) == ( - "databricks-genie-01ef9a", - f"{WS}/api/2.0/mcp/genie/01ef9a", - ) - - def test_uc_functions_splits_catalog_schema(self): - # The dot-free entry name is a slug; the URL uses the raw catalog/schema path segments. - entry_name, url = mcp.managed_mcp_server_entry("dev_cat.dev_fixture", "uc-functions", WS) - assert "." not in entry_name - assert url == f"{WS}/api/2.0/mcp/functions/dev_cat/dev_fixture" - - def test_vector_search_splits_catalog_schema(self): - entry_name, url = mcp.managed_mcp_server_entry("my_cat.my_schema", "vector-search", WS) - assert "." not in entry_name - assert url == f"{WS}/api/2.0/mcp/vector-search/my_cat/my_schema" - - def test_catalog_schema_needs_exactly_two_parts(self): - assert mcp.managed_mcp_server_entry("onlycatalog", "uc-functions", WS) is None - assert mcp.managed_mcp_server_entry("a.b.c", "uc-functions", WS) is None - - def test_app_and_unknown_types_return_none(self): - for mcp_type in ("app", "bogus"): - assert mcp.managed_mcp_server_entry("x", mcp_type, WS) is None +class TestMigrateOffManagedMcpAndSkills: + """`migrate_off_managed_mcp_and_skills` undoes MCP/skills a prior ucode applied from a managed + config, now that managed configs no longer carry them.""" - -class TestApplyManagedMcpServers: - def _managed(self, *servers): - return {"mcp_servers": list(servers)} - - def test_registers_supported_servers_for_the_launching_tool(self, monkeypatch): - applied = {} - monkeypatch.setattr(mcp, "load_state", lambda: {}) + def _patch(self, monkeypatch): + """Stub the config-file writes; capture apply calls, saved state, and notes.""" + calls: dict = {"apply": [], "saved": [], "notes": []} monkeypatch.setattr( mcp, "apply_mcp_server_changes", - lambda prev, working, clients, ws, profile=None, **kw: applied.update( - {"working": working, "clients": clients, "prev": prev} + lambda orig, working, clients, ws, profile=None, **kw: ( + calls["apply"].append({"orig": orig, "working": working, "clients": clients}) + or (orig != working) ), ) - managed = self._managed( - {"name": "system-ai-dbsql", "type": "mcp-service"}, - {"name": "databricks-sql", "type": "sql"}, - ) - registered = mcp.apply_managed_mcp_servers(managed, "claude", WS) - assert applied["clients"] == ["claude"] - assert {s["name"] for s in registered} == {"system-ai-dbsql", "databricks-sql"} - assert all(s["clients"] == ["claude"] for s in registered) - - def test_registers_genie_and_catalog_schema_types(self, monkeypatch): - applied = {} - monkeypatch.setattr(mcp, "load_state", lambda: {}) - monkeypatch.setattr( - mcp, - "apply_mcp_server_changes", - lambda prev, working, *a, **k: applied.update({"working": working}), - ) - managed = self._managed( - {"name": "01ef9a", "type": "genie-space"}, - {"name": "cat.sch", "type": "uc-functions"}, - ) - registered = mcp.apply_managed_mcp_servers(managed, "claude", WS) - names = {s["name"] for s in registered} - assert "databricks-genie-01ef9a" in names - assert any(n.startswith("databricks-functions-") for n in names) - - def test_skips_and_warns_on_unsupported_types(self, monkeypatch): - # `app` is the remaining type ucode can't rebuild from the config (needs an off-workspace - # host); it is skipped with a warning while the supported entry still registers. - warned: list[str] = [] - monkeypatch.setattr(mcp, "load_state", lambda: {}) - monkeypatch.setattr(mcp, "apply_mcp_server_changes", lambda *a, **k: None) - monkeypatch.setattr(mcp, "print_warning", lambda msg: warned.append(msg)) - managed = self._managed( - {"name": "system-ai-dbsql", "type": "mcp-service"}, - {"name": "my-app", "type": "app"}, - ) - registered = mcp.apply_managed_mcp_servers(managed, "claude", WS) - assert {s["name"] for s in registered} == {"system-ai-dbsql"} - assert warned and "my-app" in warned[0] - - def test_diffs_against_this_tools_previously_registered_servers(self, monkeypatch): - seen_prev = {} - monkeypatch.setattr( - mcp, - "load_state", - lambda: { - "managed_mcp_servers": [ - {"name": "old", "url": "u", "clients": ["claude"]}, - {"name": "other-tool", "url": "u", "clients": ["codex"]}, - ] - }, - ) - monkeypatch.setattr( - mcp, - "apply_mcp_server_changes", - lambda prev, working, *a, **k: seen_prev.update({"prev": prev}), - ) - mcp.apply_managed_mcp_servers( - self._managed({"name": "databricks-sql", "type": "sql"}), "claude", WS - ) - # Only this tool's prior servers form the diff baseline; codex's are left alone. - assert [s["name"] for s in seen_prev["prev"]] == ["old"] - - def test_no_supported_servers_does_nothing(self, monkeypatch): - monkeypatch.setattr(mcp, "load_state", lambda: {}) - monkeypatch.setattr( - mcp, - "apply_mcp_server_changes", - lambda *a, **k: pytest.fail("should not apply when nothing is registerable"), - ) - monkeypatch.setattr(mcp, "print_warning", lambda msg: None) - registered = mcp.apply_managed_mcp_servers( - self._managed({"name": "s", "type": "app"}), "claude", WS - ) - assert registered == [] - - def test_mcp_only_client_returns_empty(self, monkeypatch): - # A tool that isn't an MCP client can't have servers registered against it. - monkeypatch.setattr( - mcp, - "apply_mcp_server_changes", - lambda *a, **k: pytest.fail("should not apply for a non-client tool"), - ) - registered = mcp.apply_managed_mcp_servers( - self._managed({"name": "databricks-sql", "type": "sql"}), "not-a-client", WS - ) - assert registered == [] - - -class TestApplyManagedSkills: - def _managed(self, *names): - return {"skills": {"names": list(names)}} if names else {} - - def _skills_entry(self, servers): - return next(s for s in servers if s.get("kind") == mcp.SKILLS_MCP_KIND) - - def _patch_apply(self, monkeypatch): - """Stub out the config-file writes and report whether a change was applied.""" - monkeypatch.setattr(mcp, "save_state", lambda state: None) - monkeypatch.setattr( - mcp, "apply_mcp_server_changes", lambda orig, working, *a, **k: orig != working - ) - - def test_registers_managed_locations_for_the_launching_tool(self, monkeypatch): - self._patch_apply(monkeypatch) - state = {"workspace": WS, "mcp_servers": []} - applied = mcp.apply_managed_skills(state, self._managed("cat.sch"), "claude", WS) - assert applied == ["cat.sch"] - entry = self._skills_entry(state["mcp_servers"]) - assert entry["skill_locations"] == ["cat.sch"] - assert entry["clients"] == ["claude"] - assert state["managed_skill_locations"] == ["cat.sch"] - - def test_preserves_developer_locations_and_drops_removed_managed_ones(self, monkeypatch): - self._patch_apply(monkeypatch) - # The developer configured `mine.own`; a prior launch applied `old.managed`, now dropped from - # the config in favor of `new.managed`. + monkeypatch.setattr(mcp, "save_state", lambda state: calls["saved"].append(state.copy())) + monkeypatch.setattr(mcp, "print_note", lambda msg: calls["notes"].append(msg)) + monkeypatch.setattr(mcp, "print_warning", lambda msg: calls["notes"].append(msg)) + monkeypatch.setattr(mcp, "available_mcp_clients", lambda: list(mcp.MCP_CLIENTS)) + return calls + + def test_no_op_when_no_markers(self, monkeypatch): + calls = self._patch(monkeypatch) + state = {"workspace": WS, "mcp_servers": [{"name": "mine", "url": "u"}]} + assert mcp.migrate_off_managed_mcp_and_skills(state) is False + assert calls["apply"] == [] and calls["saved"] == [] + assert state["mcp_servers"] == [{"name": "mine", "url": "u"}] + + def test_no_op_without_workspace_keeps_markers(self, monkeypatch): + calls = self._patch(monkeypatch) + state = {"managed_mcp_servers": [{"name": "m", "clients": ["claude"]}]} + assert mcp.migrate_off_managed_mcp_and_skills(state) is False + assert state["managed_mcp_servers"] == [{"name": "m", "clients": ["claude"]}] + assert calls["apply"] == [] and calls["saved"] == [] + + def test_unregisters_a_server_from_every_client_it_was_registered_on(self, monkeypatch): + """A name repeated once per client collapses to one marker carrying both clients.""" + calls = self._patch(monkeypatch) + state = { + "workspace": WS, + "managed_mcp_servers": [ + {"name": "managed-sql", "url": "u2", "clients": ["claude"]}, + {"name": "managed-sql", "url": "u2", "clients": ["codex"]}, + ], + } + assert mcp.migrate_off_managed_mcp_and_skills(state) is True + assert len(calls["apply"]) == 1 + assert calls["apply"][0]["orig"] == [ + {"name": "managed-sql", "url": "u2", "clients": ["claude", "codex"]} + ] + assert calls["apply"][0]["working"] == [] + assert calls["apply"][0]["clients"] == ["claude", "codex"] + assert "managed_mcp_servers" not in state + assert calls["saved"] + + def test_unregisters_managed_servers_absent_from_the_developer_list(self, monkeypatch): + """The old apply path recorded managed servers only under the marker, never in mcp_servers.""" + calls = self._patch(monkeypatch) state = { "workspace": WS, - "managed_skill_locations": ["old.managed"], + "mcp_servers": [{"name": "mine", "url": "u1", "clients": ["claude"]}], + "managed_mcp_servers": [{"name": "managed-sql", "url": "u2", "clients": ["claude"]}], + } + assert mcp.migrate_off_managed_mcp_and_skills(state) is True + assert [s["name"] for s in calls["apply"][0]["orig"]] == ["managed-sql"] + assert calls["apply"][0]["working"] == [] + assert [s["name"] for s in state["mcp_servers"]] == ["mine"] + assert "managed_mcp_servers" not in state + + def test_subtracts_managed_skill_locations_keeps_developer_ones(self, monkeypatch): + calls = self._patch(monkeypatch) + state = { + "workspace": WS, + "managed_skill_locations": ["gone.managed"], "mcp_servers": [ { "name": mcp.SKILLS_MCP_SERVER_NAME, "kind": mcp.SKILLS_MCP_KIND, - "skill_locations": ["mine.own", "old.managed"], + "skill_locations": ["mine.own", "gone.managed"], "clients": ["claude"], } ], } - applied = mcp.apply_managed_skills(state, self._managed("new.managed"), "claude", WS) - assert applied == ["new.managed"] - entry = self._skills_entry(state["mcp_servers"]) - assert entry["skill_locations"] == ["mine.own", "new.managed"] - assert state["managed_skill_locations"] == ["new.managed"] - - def test_removed_managed_schema_leaves_developer_locations(self, monkeypatch): - self._patch_apply(monkeypatch) + assert mcp.migrate_off_managed_mcp_and_skills(state) is True + entry = next(s for s in state["mcp_servers"] if s.get("kind") == mcp.SKILLS_MCP_KIND) + assert entry["skill_locations"] == ["mine.own"] + assert "managed_skill_locations" not in state + assert calls["saved"] + + def test_unregisters_both_and_clears_markers(self, monkeypatch): + self._patch(monkeypatch) state = { "workspace": WS, + "managed_mcp_servers": [{"name": "managed-sql", "url": "u", "clients": ["codex"]}], "managed_skill_locations": ["gone.managed"], "mcp_servers": [ { @@ -2867,44 +2753,99 @@ def test_removed_managed_schema_leaves_developer_locations(self, monkeypatch): "kind": mcp.SKILLS_MCP_KIND, "skill_locations": ["mine.own", "gone.managed"], "clients": ["claude"], - } + }, ], } - applied = mcp.apply_managed_skills(state, self._managed(), "claude", WS) - assert applied == [] # nothing managed now, but the removal still applied - entry = self._skills_entry(state["mcp_servers"]) + assert mcp.migrate_off_managed_mcp_and_skills(state) is True + entry = next(s for s in state["mcp_servers"] if s.get("kind") == mcp.SKILLS_MCP_KIND) assert entry["skill_locations"] == ["mine.own"] - assert state["managed_skill_locations"] == [] - - def test_nothing_managed_and_none_before_is_a_noop(self, monkeypatch): - monkeypatch.setattr(mcp, "save_state", lambda state: pytest.fail("should not persist")) - monkeypatch.setattr( - mcp, "apply_mcp_server_changes", lambda *a, **k: pytest.fail("should not apply") - ) - state = {"workspace": WS, "mcp_servers": []} - assert mcp.apply_managed_skills(state, self._managed(), "claude", WS) == [] - assert "mcp_servers" in state and state["mcp_servers"] == [] + assert "managed_mcp_servers" not in state assert "managed_skill_locations" not in state - def test_unchanged_managed_set_returns_empty(self, monkeypatch): - self._patch_apply(monkeypatch) - # Build the stored entry exactly as a re-resolve would, so an unchanged config is a true - # no-op rather than differing on the derived url/auth fields. - entry = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["cat.sch"], [])[0] + def test_idempotent_second_call_is_noop(self, monkeypatch): + self._patch(monkeypatch) state = { "workspace": WS, - "managed_skill_locations": ["cat.sch"], - "mcp_servers": [entry], + "managed_mcp_servers": [{"name": "managed-sql", "url": "u", "clients": ["claude"]}], } - # Same config, same tool already registered: no change, so no note-worthy locations returned. - assert mcp.apply_managed_skills(state, self._managed("cat.sch"), "claude", WS) == [] + assert mcp.migrate_off_managed_mcp_and_skills(state) is True + assert mcp.migrate_off_managed_mcp_and_skills(state) is False - def test_non_client_tool_returns_empty(self, monkeypatch): + def test_skips_agents_whose_cli_is_no_longer_installed(self, monkeypatch): + """Unregistering shells out to the agent's CLI, so a marker naming a since-removed agent + must not reach it: the CLI is gone and the launch would die on FileNotFoundError.""" + removed: list[tuple[str, str]] = [] + monkeypatch.setattr(mcp, "available_mcp_clients", lambda: ["claude"]) monkeypatch.setattr( - mcp, "apply_mcp_server_changes", lambda *a, **k: pytest.fail("should not apply") + mcp, + "remove_client_mcp_server", + lambda client, name: removed.append((client, name)) or ["user"], ) - state = {"workspace": WS, "mcp_servers": []} - assert mcp.apply_managed_skills(state, self._managed("cat.sch"), "not-a-client", WS) == [] + monkeypatch.setattr(mcp, "save_state", lambda state: None) + monkeypatch.setattr(mcp, "print_note", lambda msg: None) + state = { + "workspace": WS, + "managed_mcp_servers": [ + {"name": "managed-sql", "url": "u", "clients": ["claude"]}, + {"name": "managed-sql", "url": "u", "clients": ["codex"]}, + ], + } + + assert mcp.migrate_off_managed_mcp_and_skills(state) is True + assert removed == [("claude", "managed-sql")] + assert "managed_mcp_servers" not in state + + def test_clears_markers_when_no_marked_agent_is_installed(self, monkeypatch): + calls = self._patch(monkeypatch) + monkeypatch.setattr(mcp, "available_mcp_clients", lambda: []) + state = { + "workspace": WS, + "managed_mcp_servers": [{"name": "managed-sql", "url": "u", "clients": ["codex"]}], + } + + assert mcp.migrate_off_managed_mcp_and_skills(state) is True + assert calls["apply"] == [] + assert "managed_mcp_servers" not in state + + def test_skills_reconcile_targets_only_installed_agents(self, monkeypatch): + calls = self._patch(monkeypatch) + monkeypatch.setattr(mcp, "available_mcp_clients", lambda: ["claude"]) + state = { + "workspace": WS, + "managed_skill_locations": ["gone.managed"], + "mcp_servers": [ + { + "name": mcp.SKILLS_MCP_SERVER_NAME, + "kind": mcp.SKILLS_MCP_KIND, + "skill_locations": ["mine.own", "gone.managed"], + "clients": ["claude", "codex"], + } + ], + } + + assert mcp.migrate_off_managed_mcp_and_skills(state) is True + assert calls["apply"][0]["clients"] == ["claude"] + entry = next(s for s in state["mcp_servers"] if s.get("kind") == mcp.SKILLS_MCP_KIND) + assert entry["skill_locations"] == ["mine.own"] + assert entry["clients"] == ["claude", "codex"] + + def test_failure_keeps_markers_and_returns_false(self, monkeypatch): + calls = self._patch(monkeypatch) + + def boom(*a, **k): + raise RuntimeError("cli exploded") + + monkeypatch.setattr(mcp, "apply_mcp_server_changes", boom) + state = { + "workspace": WS, + "managed_mcp_servers": [{"name": "managed-sql", "url": "u", "clients": ["claude"]}], + } + assert mcp.migrate_off_managed_mcp_and_skills(state) is False + assert state["managed_mcp_servers"] == [ + {"name": "managed-sql", "url": "u", "clients": ["claude"]} + ] + assert calls["saved"] == [] + assert any("retry" in n.lower() or "could not" in n.lower() for n in calls["notes"]) class TestPromptForMcpSearchSourcesExclusion: diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index c7927019..a7e79244 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -610,95 +610,6 @@ def test_none_skill_filter_downloads_everything(self, tmp_path, monkeypatch): assert (tmp_path / ".claude/skills/b/SKILL.md").exists() -class TestDownloadManagedSkillsOnLaunch: - def test_writes_missing_skills_and_returns_their_bundle_names(self, tmp_path, monkeypatch): - monkeypatch.setattr( - sd, "list_schema_skills", lambda *a, **k: ([ref("triage"), ref("pii")], None) - ) - monkeypatch.setattr( - sd, - "fetch_skill_bundle", - lambda ws, tok, c, s, leaf: ({"SKILL.md": leaf.encode()}, None), - ) - - written = sd.download_managed_skills_on_launch(WS, "token", ["main.default"], str(tmp_path)) - - assert sorted(written) == ["pii", "triage"] - assert (tmp_path / ".claude/skills/triage/SKILL.md").read_bytes() == b"triage" - assert (tmp_path / ".agents/skills/pii/SKILL.md").read_bytes() == b"pii" - - def test_skips_already_downloaded_skills_without_prompting(self, tmp_path, monkeypatch): - roots = skill_dir_roots(str(tmp_path)) - write_skill(roots, ref("triage"), {"SKILL.md": b"kept"}) - monkeypatch.setattr( - sd, "list_schema_skills", lambda *a, **k: ([ref("triage"), ref("pii")], None) - ) - fetched = [] - monkeypatch.setattr( - sd, - "fetch_skill_bundle", - lambda ws, tok, c, s, leaf: fetched.append(leaf) or ({"SKILL.md": b"new"}, None), - ) - monkeypatch.setattr(sd, "prompt_yes_no", lambda msg: pytest.fail(f"prompted: {msg}")) - - written = sd.download_managed_skills_on_launch(WS, "token", ["main.default"], str(tmp_path)) - - # Only the missing one is fetched; the existing skill is left untouched. - assert fetched == ["pii"] - assert written == ["pii"] - assert (roots[0] / "triage/SKILL.md").read_bytes() == b"kept" - - def test_nothing_missing_fetches_nothing(self, tmp_path, monkeypatch): - roots = skill_dir_roots(str(tmp_path)) - write_skill(roots, ref("triage"), {"SKILL.md": b"kept"}) - monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([ref("triage")], None)) - monkeypatch.setattr( - sd, "fetch_skill_bundle", lambda *a, **k: pytest.fail("should not fetch") - ) - - assert ( - sd.download_managed_skills_on_launch(WS, "token", ["main.default"], str(tmp_path)) == [] - ) - - def test_list_failure_warns_and_skips_location(self, tmp_path, monkeypatch, capsys): - monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([], "HTTP 404 Not Found")) - monkeypatch.setattr( - sd, "fetch_skill_bundle", lambda *a, **k: pytest.fail("should not fetch") - ) - - assert ( - sd.download_managed_skills_on_launch(WS, "token", ["main.default"], str(tmp_path)) == [] - ) - assert "Could not list workspace skills in `main.default`" in capsys.readouterr().out - - def test_bundle_failure_skips_that_skill_only(self, tmp_path, monkeypatch): - monkeypatch.setattr( - sd, "list_schema_skills", lambda *a, **k: ([ref("good"), ref("bad")], None) - ) - monkeypatch.setattr( - sd, - "fetch_skill_bundle", - lambda ws, tok, c, s, leaf: ( - ({"SKILL.md": b"ok"}, None) if leaf == "good" else (None, "HTTP 500 Server Error") - ), - ) - - written = sd.download_managed_skills_on_launch(WS, "token", ["main.default"], str(tmp_path)) - - assert written == ["good"] - assert (tmp_path / ".claude/skills/good/SKILL.md").read_bytes() == b"ok" - assert not (tmp_path / ".claude/skills/bad").exists() - - def test_malformed_location_is_skipped(self, tmp_path, monkeypatch): - monkeypatch.setattr( - sd, "list_schema_skills", lambda *a, **k: pytest.fail("should not list a bad location") - ) - - assert ( - sd.download_managed_skills_on_launch(WS, "token", ["not-a-schema"], str(tmp_path)) == [] - ) - - class TestConfigureSkillsDownloadCommand: def _stub(self, monkeypatch): calls: dict[str, object] = {}