diff --git a/README.md b/README.md index f973e0e58..ea2adab11 100644 --- a/README.md +++ b/README.md @@ -145,6 +145,16 @@ OTLP trace export with a refreshing headers helper; no background service is nee Register Databricks MCP servers for configured MCP-capable agents. Cursor Agent is MCP-only and is included when `cursor-agent` is installed: +Run `ug mcp add` without selectors to browse a table of services by name, UC location, +and status. ✓ means configured (or managed by the workspace); ✗ means not configured. +Configured and managed services stay visible as read-only rows. `[ ]`/`[x]` indicate +what you select for this operation; `[-]` marks a read-only row. + +Highlight a service to see its description, creator, and creation/update dates. Type +to filter names or UC locations, use Space to select, and Enter to save. Page Up/Down +scrolls the detail pane. Narrow terminals shorten cells and show status symbols; +the detail pane retains the full location and status. + Use `ug mcp add` to add servers without removing existing registrations: ```bash @@ -226,7 +236,15 @@ this contract does not make shared setup transactional. ## Skills Unity Catalog Skills can be registered as MCP tools or downloaded into local -agent skill directories. +agent skill directories. Run `ug skills add` without selectors to browse individual +skills in the same table and metadata pane as the MCP browser, plus the bundle name. +✓ Installed requires a recorded download of that exact UC skill with `SKILL.md` still +present at every target agent directory (`--path`, or your home directory). A matching +bundle name from another schema or metastore does not qualify. Unattributed local +bundles and incomplete installations show ✗ Not installed, with an explanation in the +detail pane. Installed skills remain selectable for updates. +Missing metadata is shown explicitly. `Created by` is the UC creator, which may +differ from the author of the bundle. ```bash # Set up the Databricks skills MCP so your agents can create and manage skills through ug. diff --git a/src/ucode/databricks.py b/src/ucode/databricks.py index 6286a0276..2f678c0e9 100644 --- a/src/ucode/databricks.py +++ b/src/ucode/databricks.py @@ -2599,10 +2599,18 @@ def _mcp_service_full_name(service: dict, required_prefix: str) -> str | None: def list_mcp_services( - workspace: str, token: str, parent: str = "system.ai", *, max_pages: int = 100 + workspace: str, + token: str, + parent: str = "system.ai", + *, + max_pages: int = 100, + metadata: dict[str, dict] | None = None, ) -> tuple[list[str], str | None]: """List UC MCP services under ``parent`` (a ``.`` ref). + ``metadata``, when provided, receives each accepted service's description and audit + fields from the same list response. No per-service requests are made. + Requests the ``BASIC`` view explicitly: only service names are needed to build the deterministic proxy URLs, and ``BASIC`` omits the source-connection resolution that makes ``FULL`` costlier (and adds Atlas load) at scale. The @@ -2639,6 +2647,11 @@ def list_mcp_services( full_name = _mcp_service_full_name(service, expected_prefix) if full_name: names.append(full_name) + if metadata is not None: + metadata[full_name] = { + key: service.get(key) + for key in ("comment", "created_by", "create_time", "update_time") + } page_token = data.get("next_page_token") or None if not page_token or page_token in seen_tokens: break @@ -3304,6 +3317,7 @@ def list_all_mcp_services( on_progress: Callable[[int, int, int], None] | None = None, on_services: Callable[[list[str]], None] | None = None, cancel_event: threading.Event | None = None, + metadata: dict[str, dict] | None = None, ) -> tuple[list[str], str | None]: """Return sorted unique MCP-service full names across every `.` in the workspace. The mcp-services API is one-schema-per-call, so this walks @@ -3323,10 +3337,15 @@ def list_all_mcp_services( names: set[str] = set() def probe(catalog, schema): - return list_mcp_services(workspace, token, f"{catalog}.{schema}") + details: dict[str, dict] = {} + kwargs = {"metadata": details} if metadata is not None else {} + result = list_mcp_services(workspace, token, f"{catalog}.{schema}", **kwargs) + return result, details def collect(result, schemas_done, schemas_total): - found, _ = result + (found, _), details = result + if metadata is not None: + metadata.update(details) new = [n for n in found if n not in names] names.update(found) if on_progress is not None: diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 6d8155763..8b15c915d 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -63,6 +63,7 @@ scrolling_checkbox, spinner, ) +from ucode.ui.interactive_picker import CatalogChoice, picker_details # Workspace-relative path fragments for the V2 AI Gateway MCP endpoints, shared by the URL-shape # checks (`_is_app_mcp_server`, `_mcp_server_location`) so the set stays in one place. @@ -464,7 +465,9 @@ def revert_mcp_configs(state: dict) -> dict[str, bool]: return results -def discover_mcp_service_names(workspace: str, profile: str | None = None) -> list[str]: +def discover_mcp_service_names( + workspace: str, profile: str | None = None, *, metadata: dict[str, dict] | None = None +) -> list[str]: """Curated `system.ai.*` MCP services. Empty list if discovery fails so callers can fall back to legacy connection discovery without surfacing every error to the picker. @@ -473,7 +476,8 @@ def discover_mcp_service_names(workspace: str, profile: str | None = None) -> li expired/invalid credential surfaces loudly instead of masquerading as an empty workspace (AIGTWY-4843).""" token = get_databricks_token(workspace, profile) - names, reason = list_mcp_services(workspace, token) + kwargs = {"metadata": metadata} if metadata is not None else {} + names, reason = list_mcp_services(workspace, token, **kwargs) raise_for_invalid_access_token(workspace, reason) return names @@ -484,6 +488,7 @@ def discover_all_mcp_service_names( on_progress: Callable[[int, int, int], None] | None = None, on_services: Callable[[list[str]], None] | None = None, cancel_event: threading.Event | None = None, + metadata: dict[str, dict] | None = None, ) -> list[str]: """All MCP services across every `.` in the workspace. This walks the workspace (see `list_all_mcp_services`) and is the workspace-wide @@ -491,12 +496,14 @@ def discover_all_mcp_service_names( the walk for live count reporting, and `on_services` to stream newly-found service names into the picker as the walk progresses.""" token = get_databricks_token(workspace, profile) + kwargs = {"metadata": metadata} if metadata is not None else {} names, reason = list_all_mcp_services( workspace, token, on_progress=on_progress, on_services=on_services, cancel_event=cancel_event, + **kwargs, ) raise_for_invalid_access_token(workspace, reason) return names @@ -653,21 +660,47 @@ def _mcp_service_choice( known_names: set[str], additive: bool, managed_names: set[str] | None = None, -) -> questionary.Choice | None: - """Picker choice for one MCP-service full name (`..`), or None to hide it. - - Shared by the initial `build_mcp_picker_choices` render and the background walk that streams - more services in, so a streamed row is built identically to an up-front one. The picker only - shows rows you can act on, so two kinds are hidden: a managed service (never ug's to add or - remove) and, under `mcp add`, an already-configured one (nothing to add). Replace mode still - lists ug's own as a removable toggle; an unregistered service is an add-choice.""" + metadata: dict | None = None, +) -> CatalogChoice: + """Picker choice for one MCP-service full name (`..`). + + Shared by initial and streamed rows. Configured services stay visible with their + status. In add mode they are read-only; managed services are always read-only. + Replace mode keeps ug's own registrations checked so they can still be removed.""" registered_as = name.replace(".", "-") - display_title = f"MCP: {name}" - if registered_as in (managed_names or set()): - return None - if registered_as in known_names: - return None if additive else _server_choice(registered_as, True, display_title) - return _add_choice(f"{MCP_SERVICE_SELECTION_PREFIX}{name}", display_title) + managed = registered_as in (managed_names or set()) + configured = registered_as in known_names or managed + status = "✓ Managed" if managed else "✓ Configured" if configured else "✗ Not configured" + details = metadata or {} + description = ( + picker_details( + name, + details.get("comment"), + created_by=details.get("created_by"), + create_time=details.get("create_time"), + update_time=details.get("update_time"), + ) + + f"\nStatus: {status}" + ) + location, _, leaf = name.rpartition(".") + return CatalogChoice( + # The title is the filter corpus in table mode. Avoid a common "MCP:" prefix, + # which would make searches such as "mcp" match every service. + title=name, + value=registered_as + if configured + else f"{MCP_ADD_PREFIX}{MCP_SERVICE_SELECTION_PREFIX}{name}", + name=leaf, + location=location, + status=status, + checked=configured and not additive and not managed, + disabled="Managed by workspace" + if managed + else "Already configured" + if configured and additive + else None, + description=description, + ) def build_mcp_picker_choices( @@ -680,6 +713,7 @@ def build_mcp_picker_choices( available_uc_functions_servers: list[dict] | None = None, additive: bool = False, managed_names: set[str] | None = None, + service_metadata: dict[str, dict] | None = None, ) -> list[questionary.Choice | questionary.Separator]: original_by_name = _servers_by_name(original_servers) managed = managed_names or set() @@ -711,7 +745,11 @@ def push(choice: questionary.Choice | None) -> None: # Picker shows the dotted UC name; state/agents store the dashed form # (see resolver). The shared helper is also used by the background walk that # streams more services in, so up-front and streamed rows match exactly. - push(_mcp_service_choice(name, known_names, additive, managed)) + push( + _mcp_service_choice( + name, known_names, additive, managed, (service_metadata or {}).get(name) + ) + ) displayed_names.add(name.replace(".", "-")) for name in available_external_names: @@ -808,6 +846,7 @@ def prompt_for_mcp_server_choices( allow_back: bool = False, additive: bool = False, managed_names: set[str] | None = None, + service_metadata: dict[str, dict] | None = None, background_loader: Callable[[Callable[[list[questionary.Choice]], None], threading.Event], None] | None = None, ) -> list[str] | None | _Back: @@ -815,8 +854,8 @@ def prompt_for_mcp_server_choices( if cancelled (Ctrl-C), or `_BACK` if `allow_back` and the user pressed Left to return to the previous wizard step. - ``additive`` (``ucode mcp add``) shows already-configured servers as - non-toggleable notes instead of pre-checked, removable boxes. + ``additive`` (``ug mcp add``) shows configured services as read-only table rows + instead of pre-checked, removable boxes. ``managed_names`` are servers managed configuration already provides; they show as non-toggleable in either mode, since there is nothing to add and ug can't remove them here. @@ -824,7 +863,7 @@ def prompt_for_mcp_server_choices( ``background_loader`` streams more choices in after the picker opens (see `scrolling_checkbox`) — used to load the workspace-wide MCP-services walk without blocking on it up front.""" - instruction = "(space to toggle, ctrl-a all, enter to save, type to filter)" + instruction = "(space toggle, ^A all, enter save, type filter, pgup/pgdn details)" if allow_back: instruction = "(space to toggle, ctrl-a all, ← back, enter to save, type to filter)" selection = scrolling_checkbox( @@ -839,11 +878,14 @@ def prompt_for_mcp_server_choices( available_uc_functions_servers, additive=additive, managed_names=managed_names, + service_metadata=service_metadata, ), style=picker_style(), instruction=instruction, allow_back=allow_back, background_loader=background_loader, + show_description=True, + table=True, ).ask() if selection is None: return None @@ -1323,23 +1365,27 @@ def _mcp_services_background_loader( never blocks the picker from opening. Deduping against already-shown rows (e.g. the fast `system.ai` list) is handled by the picker's append.""" + metadata: dict[str, dict] = {} + def loader( append: Callable[[list[questionary.Choice]], None], cancel_event: threading.Event ) -> None: def on_services(new_names: list[str]) -> None: - # A streamed service can be hidden (managed, or already configured under `mcp add`), - # in which case its choice is None — drop those so only real rows reach the picker. - new_choices = [ - choice - for name in new_names - if (choice := _mcp_service_choice(name, known_names, additive, managed_names)) - is not None - ] - if new_choices: - append(new_choices) + append( + [ + _mcp_service_choice( + name, known_names, additive, managed_names, metadata.get(name) + ) + for name in new_names + ] + ) discover_all_mcp_service_names( - workspace, profile, on_services=on_services, cancel_event=cancel_event + workspace, + profile, + on_services=on_services, + cancel_event=cancel_event, + metadata=metadata, ) return loader @@ -1347,22 +1393,24 @@ def on_services(new_names: list[str]) -> None: def _discover_selected_mcp_sources( workspace: str, profile: str | None, sources: set[str] -) -> dict[str, list]: +) -> dict[str, Any]: """Discover the picker's sources. The picker searches a single source — MCP services — so this fetches the fast curated `system.ai` list synchronously (the slow workspace-wide walk streams in afterward via the picker's background loader). The other keys stay in the returned dict as empty lists so the picker call is unchanged and can still render/remove already-registered servers of any type.""" services: list[str] = [] + metadata: dict[str, dict] = {} if MCP_SERVICES_SOURCE in sources: services = _discover_mcp_source( "MCP services", - lambda: discover_mcp_service_names(workspace, profile), + lambda: discover_mcp_service_names(workspace, profile, metadata=metadata), ) return { "external": [], "apps": [], "services": services, + "service_metadata": metadata, "genie": [], "vector_search": [], "uc_functions": [], @@ -2060,6 +2108,7 @@ def configure_mcp_command( additive=append, managed_names=managed_names, background_loader=services_loader, + service_metadata=discovered.get("service_metadata"), ) if selections is None or isinstance(selections, _Back): # No back-navigation without the source step; `_Back` can't occur, but keep the guard diff --git a/src/ucode/skills_api.py b/src/ucode/skills_api.py index 22fb5343f..9e4a72f24 100644 --- a/src/ucode/skills_api.py +++ b/src/ucode/skills_api.py @@ -51,6 +51,8 @@ class SkillRef: to preview a skill in the interactive picker. ``metastore_id``, ``skill_id``, and ``uc_update_time`` are UC attribution metadata recorded when the skill is downloaded (see ``skills_state``); the download flow never reads them. + ``created_by`` and ``create_time`` are list metadata for the browser; the UC + creator is not necessarily the author of the skill bundle. """ catalog: str @@ -61,6 +63,8 @@ class SkillRef: metastore_id: str | None = None skill_id: str | None = None uc_update_time: str | None = None + created_by: str | None = None + create_time: str | None = None @property def fqn(self) -> str: @@ -124,6 +128,8 @@ def _skill_ref(skill: dict) -> SkillRef | None: metastore_id=_non_empty_str(skill.get("metastore_id")), skill_id=_non_empty_str(skill.get("id")), uc_update_time=_non_empty_str(skill.get("update_time")), + created_by=_non_empty_str(skill.get("created_by")), + create_time=_non_empty_str(skill.get("create_time")), ) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 7aa37292b..93eee333e 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -50,6 +50,7 @@ prompt_yes_no, scrolling_checkbox, ) +from ucode.ui.interactive_picker import CatalogChoice, picker_details # `.claude/skills` (Claude) + `.agents/skills` (the alias other agents read). SKILL_BASE_DIR_NAMES = (".claude/skills", ".agents/skills") @@ -639,17 +640,60 @@ def configure_selected_skills_download_command(fqns: list[str], path: str | None # --- Interactive picker (selective download) -------------------------------- -def _skill_download_choice(ref: SkillRef, roots: list[Path]) -> questionary.Choice: - """Picker row for one skill: value is its FQN, title flags an on-disk bundle. - - On-disk skills stay selectable, since re-downloading is a legitimate update and - the existing overwrite prompt confirms it. The detail footer previews the - description behind a bold bundle-name label (the row itself shows the FQN, so the - bundle name is the one identifier not otherwise on screen). +def _skill_download_choice( + ref: SkillRef, + roots: list[Path], + *, + workspace: str | None = None, + downloads: list[dict] | None = None, +) -> questionary.Choice: + """Distinguish an attributed UC install from an unrelated bundle-name collision. + + A checkmark requires this UC source's record at every target root and SKILL.md + still on disk. Older workspace-only records are usable only in the same workspace. + Installed skills remain selectable for updates; overwrite confirmation still applies. """ - on_disk = " (on disk)" if existing_skill_on_disk(roots, ref.bundle_name) else "" - description = f"{ref.bundle_name}: {ref.description}" if ref.description else None - return questionary.Choice(title=f"{ref.fqn}{on_disk}", value=ref.fqn, description=description) + targets = {str((root / ref.bundle_name).resolve()) for root in roots} + records = list_downloaded() if downloads is None else downloads + attributed: set[str] = set() + for record in records: + if record.get("fqn") != ref.fqn or record.get("bundle_name") != ref.bundle_name: + continue + if ref.metastore_id: + if record.get("metastore_id") != ref.metastore_id: + continue + elif workspace is None or record.get("workspace", "").rstrip("/") != workspace.rstrip("/"): + continue + if ref.skill_id and record.get("skill_id") and record["skill_id"] != ref.skill_id: + continue + attributed.update(str(Path(directory).resolve()) for directory in record.get("dirs") or []) + installed = ( + bool(targets) + and targets <= attributed + and all((Path(target) / "SKILL.md").is_file() for target in targets) + ) + status = "✓ Installed" if installed else "✗ Not installed" + description = ( + picker_details( + ref.fqn, + ref.description, + created_by=ref.created_by, + create_time=ref.create_time, + update_time=ref.uc_update_time, + bundle_name=ref.bundle_name, + ) + + f"\nStatus: {status}" + ) + if not installed and existing_skill_on_disk(roots, ref.bundle_name): + description += "\nLocal bundle: Already on disk; UC source does not match or installation is incomplete" + return CatalogChoice( + title=ref.fqn, + value=ref.fqn, + name=ref.securable_name, + location=f"{ref.catalog}.{ref.schema}", + status=status, + description=description, + ) def _skills_download_background_loader( @@ -660,8 +704,19 @@ def _skills_download_background_loader( def loader( append: Callable[[list[questionary.Choice]], None], cancel_event: threading.Event ) -> str | None: + downloads_by_fqn: dict[str, list[dict]] = {} + for record in list_downloaded(): + downloads_by_fqn.setdefault(record.get("fqn", ""), []).append(record) + def on_skills(refs: list[SkillRef]) -> None: - append([_skill_download_choice(ref, roots) for ref in refs]) + append( + [ + _skill_download_choice( + ref, roots, workspace=workspace, downloads=downloads_by_fqn.get(ref.fqn, []) + ) + for ref in refs + ] + ) found, reason = list_all_skills( workspace, token, on_skills=on_skills, cancel_event=cancel_event @@ -683,11 +738,12 @@ def prompt_for_skill_download_choices( selection = scrolling_checkbox( "Skills:", choices=[], - instruction="(space to toggle, ctrl-a all, enter to save, type to filter)", + instruction="(space toggle, ^A all, enter save, type filter, pgup/pgdn details)", style=picker_style(), background_loader=background_loader, loading_noun="skills", show_description=True, + table=True, ).ask() if selection is None: return None diff --git a/src/ucode/ui/interactive_picker.py b/src/ucode/ui/interactive_picker.py index d76b47f9b..0e71c58cf 100644 --- a/src/ucode/ui/interactive_picker.py +++ b/src/ucode/ui/interactive_picker.py @@ -12,10 +12,11 @@ import time from collections.abc import Callable from contextlib import suppress +from datetime import UTC from typing import Any import questionary -from prompt_toolkit.application import Application +from prompt_toolkit.application import Application, get_app from prompt_toolkit.filters import Condition, IsDone from prompt_toolkit.key_binding import KeyBindings from prompt_toolkit.keys import Keys @@ -23,10 +24,13 @@ from prompt_toolkit.layout.controls import FormattedTextControl from prompt_toolkit.layout.dimension import Dimension from prompt_toolkit.shortcuts import PromptSession +from prompt_toolkit.utils import get_cwidth from questionary.prompts.common import InquirerControl from questionary.question import Question from questionary.styles import merge_styles_default +from ucode.time_utils import parse_update_time + PICKER_VISIBLE_ROWS = 10 # Cap the highlighted-row description preview so a long description stays within the @@ -56,6 +60,51 @@ def _description_footer_tokens(description: str) -> list[tuple[str, str]]: return [("bold", f"{label}{sep}"), ("class:instruction", _description_preview(body))] +def picker_details( + name: str, + description: object, + *, + created_by: object = None, + create_time: object = None, + update_time: object = None, + bundle_name: str | None = None, +) -> str: + """Build plain-text detail lines from list metadata; never fetch bundles or credentials. + + Creator is labeled as such: UC does not expose the bundle's author. Missing fields + remain explicit rather than implying that the owner is the author. + """ + + def text(value: object, fallback: str = "Not available") -> str: + return " ".join(value.split()) if isinstance(value, str) and value.strip() else fallback + + def date(value: object) -> str: + parsed = parse_update_time(value) if isinstance(value, str) else None + return parsed.astimezone(UTC).strftime("%Y-%m-%d") if parsed else "Not available" + + lines = [f"Full name: {text(name)}"] + if bundle_name: + lines.append(f"Bundle: {text(bundle_name)}") + lines.extend( + [ + f"Description: {text(description, 'No description provided')}", + f"Created by: {text(created_by)}", + f"Created: {date(create_time)}", + f"Updated: {date(update_time)}", + ] + ) + return "\n".join(lines) + + +def _detail_tokens(description: str) -> list[tuple[str, str]]: + tokens: list[tuple[str, str]] = [] + for index, line in enumerate(description.splitlines()): + if index: + tokens.append(("", "\n")) + tokens.extend(_description_footer_tokens(line)) + return tokens + + class _Back: """Sentinel type: a wizard step returns the `_BACK` instance when the user presses Left (←) to go back. Distinct from None (cancel) and [] (empty).""" @@ -76,6 +125,63 @@ def picker_style() -> questionary.Style: ) +class CatalogChoice(questionary.Choice): + """A searchable choice whose table cells are independent of its selection state.""" + + def __init__( + self, + title: str, + value: str, + *, + name: str, + location: str, + status: str, + checked: bool = False, + disabled: str | None = None, + description: str | None = None, + ): + super().__init__( + title, value=value, checked=checked, disabled=disabled, description=description + ) + self.name = name + self.location = location + self.status = status + + +def _table_cell(value: str, width: int) -> str: + value = " ".join(value.split()) + if get_cwidth(value) > width: + clipped = "" + for char in value: + if get_cwidth(clipped + char) > width - 1: + break + clipped += char + value = clipped + "…" + return value + " " * max(0, width - get_cwidth(value)) + + +def _table_columns(width: int) -> tuple[int, int, int]: + # Keep the table compact on wide terminals. Stretching short values across the full + # viewport makes related cells harder to scan and leaves the status far from the name. + width = min(width, 96) + status = 16 if width >= 60 else 6 + remaining = max(2, width - 9 - 4 - status) + return remaining // 2, remaining - remaining // 2, status + + +def _table_cells(name: str, location: str, status: str, width: int) -> str: + name_width, location_width, status_width = _table_columns(width) + if width < 60 and status.startswith(("✓", "✗")): + status = status[0] + return " ".join( + [ + _table_cell(name, name_width), + _table_cell(location, location_width), + _table_cell(status, status_width), + ] + ) + + class StreamingInquirerControl(InquirerControl): """`InquirerControl` that tolerates an empty or all-disabled choice list. @@ -87,6 +193,10 @@ class StreamingInquirerControl(InquirerControl): already-configured (disabled) rows. Default the cursor and treat "nothing selectable" as valid so construction, rendering, and navigation don't crash.""" + def __init__(self, choices, *, table: bool = False, **kwargs): + self.table = table + super().__init__(choices, **kwargs) + def is_selection_valid(self) -> bool: if getattr(self, "pointed_at", None) is None: self.pointed_at = 0 @@ -99,6 +209,10 @@ def is_selection_valid(self) -> bool: return True if self.pointed_at >= len(self.choices): return False + if self.table and isinstance(self.get_pointed_at(), CatalogChoice): + # Configured/managed rows stay browsable for their details; Space and + # select-all still exclude their disabled choices. + return True return super().is_selection_valid() def _get_choice_tokens(self): @@ -106,17 +220,67 @@ def _get_choice_tokens(self): # empty list. Render nothing when there are no rows (the picker is still streaming them in). if not self.filtered_choices: return [] - return super()._get_choice_tokens() + if not self.table: + return super()._get_choice_tokens() + tokens = [] + width = get_app().output.get_size().columns + for index, choice in enumerate(self.filtered_choices): + if index: + tokens.append(("", "\n")) + pointed = index == self.pointed_at + tokens.append(("class:pointer", "› " if pointed else " ")) + if pointed: + tokens.append(("[SetCursorPosition]", "")) + selected = choice.value in self.selected_options + checkbox = "[-]" if choice.disabled else "[x]" if selected else "[ ]" + tokens.append(("class:text", checkbox + " ")) + if isinstance(choice, CatalogChoice): + name, location, status = choice.name, choice.location, choice.status + else: + # Legacy MCP registrations have no UC location, but remain removable. + name = str(choice.title) + location = "—" + status = "✓ Configured" if choice.checked else "✗ Not configured" + style = ( + "class:disabled" + if choice.disabled + else "class:highlighted" + if pointed + else "class:text" + ) + tokens.append((style, _table_cells(name, location, status, width))) + return tokens def merge_new_choices( existing: list[questionary.Choice | questionary.Separator], new_choices: list[questionary.Choice], -) -> list[questionary.Choice]: - """Return the choices from ``new_choices`` not already present in ``existing`` (compared by - Choice value). Used to dedupe background-streamed picker rows against what's already shown.""" - shown = {c.value for c in existing if isinstance(c, questionary.Choice)} - return [c for c in new_choices if isinstance(c, questionary.Choice) and c.value not in shown] +) -> tuple[list[questionary.Choice | questionary.Separator], int]: + """Merge streamed choices by value and return the rows plus the number newly discovered. + + A rich catalog row replaces a plain initial row with the same value. This preserves the + selection (which is value-based) while adding the streamed location and detail metadata. + """ + incoming = {choice.value: choice for choice in new_choices} + merged: list[questionary.Choice | questionary.Separator] = [] + seen: set[Any] = set() + added = 0 + for choice in existing: + if isinstance(choice, questionary.Choice) and choice.value in incoming: + replacement = incoming[choice.value] + if isinstance(replacement, CatalogChoice) and not isinstance(choice, CatalogChoice): + merged.append(replacement) + else: + merged.append(choice) + seen.add(choice.value) + else: + merged.append(choice) + for choice in new_choices: + if choice.value not in seen: + merged.append(choice) + seen.add(choice.value) + added += 1 + return merged, added def scrolling_checkbox( @@ -131,6 +295,7 @@ def scrolling_checkbox( | None = None, loading_noun: str = "MCP services", show_description: bool = False, + table: bool = False, ) -> Question: """Multi-select checkbox picker. @@ -141,6 +306,7 @@ def scrolling_checkbox( "Loading more {loading_noun}, this might take a few seconds..." count while it runs. ``show_description`` adds a footer previewing the highlighted row's ``Choice.description``. + Newline-separated metadata retains its labels; Page Up/Down scroll the detail pane. It's a separate window rather than questionary's inline ``show_description`` because the choices window is sized to the row count, so an inline line would be clipped.""" merged_style = merge_styles_default( @@ -155,6 +321,7 @@ def scrolling_checkbox( choices, pointer="›", show_description=False, + table=table, ) # Live loading state for the background-loader footer (see below). loading: dict[str, Any] = {"active": background_loader is not None, "found": 0, "message": None} @@ -176,10 +343,19 @@ def perform_validation() -> bool: control.error_message = None return True + def detail_height() -> int: + return min(8, max(1, app.output.get_size().rows // 3)) + + def visible_choice_rows() -> int: + # Reserve the prompt and optional loading/search/scroll footers. Re-evaluate + # on resize so the detail pane cannot push the choices out of a short terminal. + reserved = 5 + int(table) + (detail_height() + 1 if show_description else 0) + return min(PICKER_VISIBLE_ROWS, max(1, app.output.get_size().rows - reserved)) + @Condition def has_more_choices() -> bool: # Live so the scroll hint appears as background-loaded rows stream in. - return len(control.choices) > PICKER_VISIBLE_ROWS + return len(control.choices) > visible_choice_rows() @Condition def show_loading_footer() -> bool: @@ -210,11 +386,13 @@ def pointed_description() -> str | None: description = getattr(pointed, "description", None) return description if isinstance(description, str) and description else None + detail_line = {"offset": 0} + def description_tokens() -> list[tuple[str, str]]: description = pointed_description() if description is None: return [] - return _description_footer_tokens(description) + return _detail_tokens("\n".join(description.splitlines()[detail_line["offset"] :])) @Condition def has_description() -> bool: @@ -231,6 +409,26 @@ def has_description() -> bool: height=Dimension.exact(1), content=FormattedTextControl(get_prompt_tokens), ), + ConditionalContainer( + Window( + height=Dimension.exact(1), + content=FormattedTextControl( + lambda: [ + ( + "bold", + " Select " + + _table_cells( + "Name", + "UC location", + "Status", + app.output.get_size().columns, + ), + ) + ] + ), + ), + filter=Condition(lambda: table) & ~IsDone(), + ), ConditionalContainer( # Height tracks the live choice count (capped at the visible max) so the # window grows as background-loaded rows stream in, with no blank gap when @@ -238,7 +436,7 @@ def has_description() -> bool: Window( control, height=lambda: Dimension.exact( - min(PICKER_VISIBLE_ROWS, max(1, len(control.choices))) + min(visible_choice_rows(), max(1, len(control.choices))) ), ), filter=~IsDone(), @@ -277,8 +475,13 @@ def has_description() -> bool: [ Window(height=Dimension.exact(1)), Window( - height=Dimension.exact(2), - content=FormattedTextControl(description_tokens), + height=lambda: Dimension( + min=1, preferred=detail_height(), max=detail_height() + ), + content=FormattedTextControl( + description_tokens, + show_cursor=False, + ), wrap_lines=True, get_line_prefix=lambda line, wrap: _DESCRIPTION_INDENT, ), @@ -332,6 +535,7 @@ def _(_event: Any) -> None: perform_validation() def move_cursor_down(event: Any) -> None: + detail_line["offset"] = 0 if control.choice_count == 0: return control.select_next() @@ -342,6 +546,7 @@ def move_cursor_down(event: Any) -> None: tries += 1 def move_cursor_up(event: Any) -> None: + detail_line["offset"] = 0 if control.choice_count == 0: return control.select_previous() @@ -351,6 +556,7 @@ def move_cursor_up(event: Any) -> None: tries += 1 def search_filter(event: Any) -> None: + detail_line["offset"] = 0 control.add_search_character(event.key_sequence[0].key) for character in string.printable: @@ -364,6 +570,19 @@ def search_filter(event: Any) -> None: bindings.add(Keys.ControlN, eager=True)(move_cursor_down) bindings.add(Keys.ControlP, eager=True)(move_cursor_up) + if show_description: + + @bindings.add(Keys.PageDown, eager=True) + def _(_event: Any) -> None: + description = pointed_description() or "" + detail_line["offset"] = min( + max(0, len(description.splitlines()) - 1), detail_line["offset"] + 3 + ) + + @bindings.add(Keys.PageUp, eager=True) + def _(_event: Any) -> None: + detail_line["offset"] = max(0, detail_line["offset"] - 3) + @bindings.add(Keys.ControlM, eager=True) def _(event: Any) -> None: control.submission_attempted = True @@ -412,10 +631,10 @@ def append(new_choices: list[questionary.Choice]) -> None: def apply() -> None: # On the UI thread: rebind choices (atomic; render reads the list live) and repaint. # Selections track by value, so appended rows never disturb checkboxes/scroll/filter. - additions = merge_new_choices(control.choices, new_choices) - if additions: - control.choices = [*control.choices, *additions] - loading["found"] += len(additions) + merged, added = merge_new_choices(control.choices, new_choices) + if merged != control.choices: + control.choices = merged + loading["found"] += added app.invalidate() run_on_loop(apply) diff --git a/tests/README.md b/tests/README.md index 1a13dfa0b..7e7953b74 100644 --- a/tests/README.md +++ b/tests/README.md @@ -1,5 +1,13 @@ # Test suites and user journeys +MCP/Skills browsing has component coverage in `tests/test_catalog_browsing.py` +(repository-relative path): paginated BASIC metadata, streamed rows, missing metadata, +UTC dates, keyboard selection/filtering/cancellation, loader cancellation, and rendered +80×24/40×12 table rendering and detail scrolling. Status coverage distinguishes selection +from configuration/installation, protects read-only managed MCP rows, and checks Skills +attribution against source identity, target directories, and surviving files. These checks +do not establish live workspace discovery or agent invocation coverage. + Integration runs a freshly installed ug wheel/release, exact real Claude/Codex (and optionally OpenCode) versions, and the existing real e2e workspace. It has no application imports, mocks, monkeypatching, fake binaries/services, or fabricated ug state. diff --git a/tests/integration/README.md b/tests/integration/README.md index 0e07feb22..3d8cd1307 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -13,6 +13,14 @@ percent consumed, spend, and threshold are treated like missing optional fields, individually or together, without discarding valid statuses. Redundant client error variants are reduced to one opaque passthrough case; later-page failure checks remain. +MCP/Skills browsing has component coverage in `tests/test_catalog_browsing.py` +(repository-relative path): paginated BASIC metadata, streamed rows, missing metadata, +UTC dates, keyboard selection/filtering/cancellation, loader cancellation, and rendered +80×24/40×12 table rendering and detail scrolling. Status coverage distinguishes selection +from configuration/installation, protects read-only managed MCP rows, and checks Skills +attribution against source identity, target directories, and surviving files. These checks +do not establish live workspace discovery or agent invocation coverage. + Claude answer delivery has offline transcript and subprocess coverage in `../test_claude_answer_delivery.py`, including stdout preservation for text, JSON and streaming JSON when UG redirects diagnostics to stderr. Live model/gateway selection diff --git a/tests/test_catalog_browsing.py b/tests/test_catalog_browsing.py new file mode 100644 index 000000000..ac206e500 --- /dev/null +++ b/tests/test_catalog_browsing.py @@ -0,0 +1,463 @@ +"""Component coverage for metadata discovery and the shared MCP/Skills browser.""" + +import asyncio +import threading + +import pytest +import questionary +from prompt_toolkit.application import create_app_session +from prompt_toolkit.input import create_pipe_input +from prompt_toolkit.output import DummyOutput + +from ucode import databricks, mcp, skills_api, skills_download +from ucode.ui.interactive_picker import ( + CatalogChoice, + StreamingInquirerControl, + _detail_tokens, + picker_details, + picker_style, + scrolling_checkbox, +) + + +def test_mcp_listing_retains_metadata_without_extra_requests(monkeypatch): + calls = [] + pages = iter( + [ + { + "mcp_services": [ + { + "name": "mcp-services/system.ai.search", + "comment": "Search docs", + "created_by": "creator@example.com", + "create_time": "2026-09-01T00:00:00Z", + }, + {"name": "mcp-services/other.schema.hidden", "comment": "Wrong parent"}, + { + "name": "mcp-services/system.ai.inactive", + "config": {"connection": {"status": "FAILED"}}, + }, + ], + "next_page_token": "next", + }, + {"mcp_services": [{"name": "mcp-services/system.ai.empty"}]}, + ] + ) + + def get(url, token, timeout): + calls.append(url) + return next(pages), None + + monkeypatch.setattr(databricks, "_http_get_json", get) + metadata = {} + names, reason = databricks.list_mcp_services("https://example.com", "token", metadata=metadata) + assert reason is None + assert names == ["system.ai.empty", "system.ai.search"] + assert set(metadata) == set(names) + assert metadata["system.ai.search"]["comment"] == "Search docs" + assert metadata["system.ai.search"]["created_by"] == "creator@example.com" + assert len(calls) == 2 + assert all("view=BASIC" in url for url in calls) + assert "page_token=next" in calls[1] + + +def test_streamed_mcp_metadata_is_ready_before_rows_are_emitted(monkeypatch): + metadata = {} + emitted = [] + + def walk(workspace, token, *, probe, collect, **kwargs): + for index, schema in enumerate(["one", "two"]): + collect(probe("cat", schema), index + 1, 2) + return None + + def listing(workspace, token, parent, *, metadata): + name = parent + ".tool" + metadata[name] = {"comment": "Tool in " + parent} + return [name], None + + def on_services(names): + emitted.extend((name, metadata[name]["comment"]) for name in names) + + monkeypatch.setattr(databricks, "walk_catalog_schemas", walk) + monkeypatch.setattr(databricks, "list_mcp_services", listing) + names, reason = databricks.list_all_mcp_services( + "https://example.com", + "token", + metadata=metadata, + on_services=on_services, + ) + assert reason is None + assert names == ["cat.one.tool", "cat.two.tool"] + assert emitted == [("cat.one.tool", "Tool in cat.one"), ("cat.two.tool", "Tool in cat.two")] + + +def test_skill_metadata_survives_parsing_and_choice_construction(tmp_path): + ref = skills_api._skill_ref( + { + "name": "skills/cat.sch.securable", + "bundle_name": "bundle", + "description": "Analyze tickets", + "finalize_time": "2026-09-01T00:00:00Z", + "created_by": "creator@example.com", + "create_time": "2026-08-01T00:00:00Z", + "update_time": "2026-09-01T00:00:00Z", + } + ) + assert ref is not None + choice = skills_download._skill_download_choice(ref, [tmp_path]) + assert choice.value == "cat.sch.securable" + assert "Full name: cat.sch.securable" in choice.description + assert "Bundle: bundle" in choice.description + assert "Description: Analyze tickets" in choice.description + assert "Created by: creator@example.com" in choice.description + assert "Created: 2026-08-01" in choice.description + assert "Updated: 2026-09-01" in choice.description + + +def test_initial_and_streamed_mcp_choices_have_identical_details(monkeypatch): + details = {"comment": "Read docs", "created_by": "creator@example.com"} + initial = mcp.build_mcp_picker_choices( + [], + [], + [], + [], + ["cat.sch.docs"], + service_metadata={"cat.sch.docs": details}, + )[0] + + def discover(workspace, profile, *, on_services, metadata, **kwargs): + metadata["cat.sch.docs"] = details + on_services(["cat.sch.docs"]) + + monkeypatch.setattr(mcp, "discover_all_mcp_service_names", discover) + streamed = [] + mcp._mcp_services_background_loader("https://example.com", None, set(), True)( + streamed.extend, + threading.Event(), + ) + assert streamed[0].description == initial.description + assert streamed[0].value == initial.value + assert "Created by: creator@example.com" in initial.description + managed = mcp.build_mcp_picker_choices( + [], + [], + [], + [], + ["cat.sch.docs"], + managed_names={"cat-sch-docs"}, + service_metadata={"cat.sch.docs": details}, + )[0] + assert managed.disabled == "Managed by workspace" + assert managed.status == "✓ Managed" + + +def test_details_preserve_fields_and_normalize_dates(): + details = picker_details( + "cat.sch.tool", + "Long description. " * 100, + created_by="creator\n@example.com", + create_time="2026-09-01T03:00:00+03:00", + update_time="invalid", + ) + rendered = "".join(token for _, token in _detail_tokens(details)) + assert "…\nCreated by: creator @example.com" in rendered + assert "Created: 2026-09-01" in rendered + assert "Updated: Not available" in rendered + missing = picker_details("cat.sch.tool", {"unexpected": "value"}, created_by=123) + assert "Description: No description provided" in missing + assert "Created by: Not available" in missing + + +@pytest.mark.parametrize( + "keys, expected", + [ + (" \x1b[B \r", ["first", "second"]), + ("second \r", ["second"]), + ("\x01\r", ["first", "second"]), + ("\r", []), + ("\x03", None), + ], +) +def test_browser_keyboard_preserves_selection_and_cancellation(keys, expected): + async def run(): + with create_pipe_input() as pipe, create_app_session(input=pipe, output=DummyOutput()): + question = scrolling_checkbox( + "Browse", + [ + CatalogChoice( + name, + value=name, + name=name, + location="cat.sch", + status="✗ Not installed", + description=picker_details(name, name), + ) + for name in ["first", "second"] + ], + "space select, enter save", + picker_style(), + show_description=True, + table=True, + ) + task = asyncio.create_task(question.ask_async()) + await asyncio.sleep(0.05) + pipe.send_text(keys) + result = await asyncio.wait_for(task, 2) + assert result == expected + + asyncio.run(run()) + + +def test_empty_browser_streams_choices_with_details_and_cancels_loader(): + cancel = [] + + def loader(append, cancel_event): + cancel.append(cancel_event) + append([questionary.Choice("streamed", value="value", description="Details: streamed")]) + + async def run(): + with create_pipe_input() as pipe, create_app_session(input=pipe, output=DummyOutput()): + question = scrolling_checkbox( + "Browse", + [], + "space select", + picker_style(), + show_description=True, + table=True, + background_loader=loader, + ) + task = asyncio.create_task(question.ask_async()) + control = next( + c + for c in question.application.layout.find_all_controls() + if isinstance(c, StreamingInquirerControl) + ) + async with asyncio.timeout(2): + while not control.choices: + await asyncio.sleep(0.01) + assert control.choices[0].description == "Details: streamed" + pipe.send_text(" \r") + assert await asyncio.wait_for(task, 2) == ["value"] + assert cancel[0].is_set() + + asyncio.run(run()) + + +@pytest.mark.parametrize("columns, rows", [(80, 24), (40, 12)]) +def test_rendered_browser_scrolls_details_in_small_terminals(columns, rows): + """Render the real application and assert terminal contents and saved values.""" + from io import StringIO + + import pyte + from prompt_toolkit.data_structures import Size + from prompt_toolkit.output.vt100 import Vt100_Output + + async def run(): + stdout = StringIO() + output = Vt100_Output( + stdout, + lambda: Size(rows=rows, columns=columns), + term="xterm-256color", + enable_cpr=False, + ) + screen = pyte.Screen(columns, rows) + stream = pyte.Stream(screen) + offset = 0 + + def rendered(): + nonlocal offset + raw = stdout.getvalue() + stream.feed(raw[offset:]) + offset = len(raw) + return "\n".join(screen.display) + + with create_pipe_input() as pipe, create_app_session(input=pipe, output=output): + choices = [ + CatalogChoice( + f"cat.sch.tool{index}", + value=str(index), + name=f"tool{index}", + location="cat.sch", + status="✗ Not installed", + description=picker_details( + f"cat.sch.tool{index}", + "Read documents. " * 40, + created_by="team@example.com", + update_time="2026-10-15T12:30:00Z", + ), + ) + for index in range(20) + ] + question = scrolling_checkbox( + "Browse", + choices, + "space select, enter save", + picker_style(), + show_description=True, + table=True, + ) + task = asyncio.create_task(question.ask_async()) + try: + await asyncio.sleep(0.1) + assert "Select" in rendered() + assert "UC location" in rendered() + assert "tool0" in rendered() + assert "[ ]" in rendered() + assert "✗" in rendered() + pipe.send_text("\x1b[6~") # One Page Down visibly moves the detail pane + await asyncio.sleep(0.1) + text = rendered() + assert "Updated:" in text + assert "2026-10-15" in text + pipe.send_text("\x1b[B") # Next choice resets details to the top + await asyncio.sleep(0.1) + assert "Full name: cat.sch.tool1" in rendered() + pipe.send_text(" \r") + assert await asyncio.wait_for(task, 2) == ["1"] + finally: + if not task.done(): + pipe.send_text("\x03") + await asyncio.wait_for(task, 2) + + asyncio.run(run()) + + +def _install_browser_fixture(tmp_path, *, metastore_id="meta", workspace="https://example.com"): + from ucode.skills_state import SkillInstall, record_downloads + + ref = skills_api.SkillRef("cat", "sch", "triage", "triage", metastore_id=metastore_id) + roots = skills_download.skill_dir_roots(str(tmp_path)) + skills_download.write_skill(roots, ref, {"SKILL.md": b"skill"}) + record_downloads( + [ + SkillInstall( + fqn=ref.fqn, + bundle_name=ref.bundle_name, + workspace=workspace, + scope="user", + base=str(tmp_path), + dirs=tuple(str(root / ref.bundle_name) for root in roots), + metastore_id=metastore_id, + ) + ] + ) + return ref, roots + + +def test_installed_marker_requires_exact_source_at_target_path(tmp_path): + from dataclasses import replace + + ref, roots = _install_browser_fixture(tmp_path) + choice = skills_download._skill_download_choice(ref, roots) + assert choice.status == "✓ Installed" + assert choice.checked is False # installed does not mean selected to download + assert choice.disabled is None # updates are still allowed + same_bundle_other_schema = replace(ref, schema="other") + other = skills_download._skill_download_choice(same_bundle_other_schema, roots) + assert other.status == "✗ Not installed" + assert "Local bundle: Already on disk" in other.description + assert ( + skills_download._skill_download_choice( + replace(ref, metastore_id="different"), + roots, + ).status + == "✗ Not installed" + ) + # The same source installed elsewhere must not mark this target installed. + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + other_roots = skills_download.skill_dir_roots(str(elsewhere)) + skills_download.write_skill(other_roots, ref, {"SKILL.md": b"unattributed"}) + assert skills_download._skill_download_choice(ref, other_roots).status == "✗ Not installed" + + +def test_installed_marker_requires_files_in_every_target_root(tmp_path): + ref, roots = _install_browser_fixture(tmp_path) + (roots[1] / ref.bundle_name / "SKILL.md").unlink() + choice = skills_download._skill_download_choice(ref, roots) + assert choice.status == "✗ Not installed" + assert "installation is incomplete" in choice.description + + +def test_workspace_only_attribution_is_not_shared_across_workspaces(tmp_path): + ref, roots = _install_browser_fixture(tmp_path, metastore_id=None) + assert ( + skills_download._skill_download_choice( + ref, + roots, + workspace="https://example.com/", + ).status + == "✓ Installed" + ) + assert ( + skills_download._skill_download_choice( + ref, + roots, + workspace="https://other.example.com", + ).status + == "✗ Not installed" + ) + assert skills_download._skill_download_choice(ref, roots).status == "✗ Not installed" + + +def test_skill_loader_reads_attribution_once_for_all_streamed_batches(monkeypatch, tmp_path): + ref, roots = _install_browser_fixture(tmp_path) + calls = [] + real_list = skills_download.list_downloaded + monkeypatch.setattr(skills_download, "list_downloaded", lambda: calls.append(1) or real_list()) + + def list_skills(workspace, token, *, on_skills, **kwargs): + on_skills([ref]) + on_skills([skills_api.SkillRef("cat", "other", "triage", "triage", metastore_id="meta")]) + return [], None + + monkeypatch.setattr(skills_download, "list_all_skills", list_skills) + choices = [] + skills_download._skills_download_background_loader("https://example.com", "token", roots)( + choices.extend, + threading.Event(), + ) + assert calls == [1] + assert [choice.status for choice in choices] == ["✓ Installed", "✗ Not installed"] + + +@pytest.mark.parametrize( + "keys, expected", + [ + ("managed \r", []), + ("existing \r", []), + ("\x01\r", ["add:mcp-service:cat.sch.new"]), + ], +) +def test_read_only_mcp_rows_are_browsable_but_never_selected(keys, expected): + choices = mcp.build_mcp_picker_choices( + [], + [], + [], + [{"name": "cat-sch-existing", "url": "https://example.com"}], + ["cat.sch.managed", "cat.sch.existing", "cat.sch.new"], + additive=True, + managed_names={"cat-sch-managed"}, + ) + + async def run(): + with create_pipe_input() as pipe, create_app_session(input=pipe, output=DummyOutput()): + question = scrolling_checkbox( + "MCP", + choices, + "Select", + picker_style(), + table=True, + show_description=True, + ) + task = asyncio.create_task(question.ask_async()) + await asyncio.sleep(0.05) + pipe.send_text(keys) + assert await asyncio.wait_for(task, 2) == expected + assert [choice.status for choice in choices] == [ + "✓ Managed", + "✓ Configured", + "✗ Not configured", + ] + + asyncio.run(run()) diff --git a/tests/test_interactive_picker.py b/tests/test_interactive_picker.py index 5d332a844..2393cd34b 100644 --- a/tests/test_interactive_picker.py +++ b/tests/test_interactive_picker.py @@ -6,6 +6,7 @@ from ucode.ui.interactive_picker import ( _DESCRIPTION_PREVIEW_CHARS, + CatalogChoice, StreamingInquirerControl, _description_footer_tokens, _description_preview, @@ -18,8 +19,25 @@ def test_merge_new_choices_dedupes_by_value(): # listed (e.g. from a fast up-front pass) isn't added twice. a = questionary.Choice(title="A", value="cat.sch.a") b = questionary.Choice(title="B", value="cat.sch.b") - merged = merge_new_choices([a], [a, b]) - assert [c.value for c in merged] == ["cat.sch.b"] + merged, added = merge_new_choices([a], [a, b]) + assert [c.value for c in merged] == ["cat.sch.a", "cat.sch.b"] + assert added == 1 + + +def test_merge_new_choices_enriches_plain_row_without_losing_its_value(): + plain = questionary.Choice(title="cat-sch-a", value="cat-sch-a", checked=True) + rich = CatalogChoice( + "cat.sch.a", + value="cat-sch-a", + name="a", + location="cat.sch", + status="✓ Configured", + checked=True, + description="Full name: cat.sch.a", + ) + merged, added = merge_new_choices([plain], [rich]) + assert merged == [rich] + assert added == 0 class TestStreamingInquirerControl: diff --git a/tests/test_mcp.py b/tests/test_mcp.py index f57147a3e..621799926 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -451,7 +451,7 @@ def fake_checkbox(*args, **kwargs): assert "Databricks SQL" not in choice_text assert "Built-in AI tools" not in choice_text assert checkbox_calls[0]["kwargs"]["instruction"] == ( - "(space to toggle, ctrl-a all, enter to save, type to filter)" + "(space toggle, ^A all, enter save, type filter, pgup/pgdn details)" ) def test_prompt_returns_none_when_cancelled(self, monkeypatch): @@ -608,7 +608,9 @@ def _patch_mcp_choices(monkeypatch, *values: str, categories: set[str] | None = ) # Stub the always-on discoveries so configure_mcp_command tests don't hit # real APIs. Individual tests override these after calling the helper. - monkeypatch.setattr(mcp, "discover_mcp_service_names", lambda workspace, profile=None: []) + monkeypatch.setattr( + mcp, "discover_mcp_service_names", lambda workspace, profile=None, **kwargs: [] + ) monkeypatch.setattr( mcp, "discover_all_mcp_service_names", @@ -1094,7 +1096,12 @@ def test_workspace_wide_walk_streams_services_into_picker(self, monkeypatch): walk_calls: list[str] = [] def fake_walk( - workspace, profile=None, on_progress=None, on_services=None, cancel_event=None + workspace, + profile=None, + on_progress=None, + on_services=None, + cancel_event=None, + metadata=None, ): walk_calls.append(workspace) if on_services is not None: @@ -1114,17 +1121,19 @@ def fake_walk( def test_mcp_service_choice_known_vs_unknown(self): # Unregistered -> an add-choice; already-registered -> a removable toggle in replace - # mode, and hidden (None) under `mcp add` since there is nothing to add. + # mode, and read-only under `mcp add` since there is nothing to add. add = mcp._mcp_service_choice("mycat.sch.weather", set(), additive=False) assert ( add.value == f"{mcp.MCP_ADD_PREFIX}{mcp.MCP_SERVICE_SELECTION_PREFIX}mycat.sch.weather" ) toggle = mcp._mcp_service_choice("mycat.sch.weather", {"mycat-sch-weather"}, additive=False) assert toggle.value == "mycat-sch-weather" and toggle.checked - assert ( - mcp._mcp_service_choice("mycat.sch.weather", {"mycat-sch-weather"}, additive=True) - is None + readonly = mcp._mcp_service_choice( + "mycat.sch.weather", {"mycat-sch-weather"}, additive=True ) + assert readonly.disabled == "Already configured" + assert readonly.checked is False + assert readonly.status == "✓ Configured" def test_skips_slow_walks_unless_source_selected(self, monkeypatch): """Vector Search and UC functions walk the workspace and are OFF by @@ -1193,7 +1202,9 @@ def test_preserves_skills_connection_and_hides_it_from_picker(self, monkeypatch) monkeypatch.setattr(mcp, "ensure_databricks_auth", lambda workspace, profile=None: None) monkeypatch.setattr(mcp, "available_mcp_clients", lambda: ["claude"]) monkeypatch.setattr(mcp, "discover_app_mcp_servers", lambda workspace, profile=None: []) - monkeypatch.setattr(mcp, "discover_mcp_service_names", lambda workspace, profile=None: []) + monkeypatch.setattr( + mcp, "discover_mcp_service_names", lambda workspace, profile=None, **kwargs: [] + ) monkeypatch.setattr( mcp, "prompt_for_mcp_server_choices", @@ -1418,7 +1429,7 @@ def test_continues_when_mcp_service_discovery_fails(self, monkeypatch, capsys): monkeypatch.setattr( mcp, "discover_mcp_service_names", - lambda workspace, profile=None: (_ for _ in ()).throw( + lambda workspace, profile=None, **kwargs: (_ for _ in ()).throw( RuntimeError("permission denied") ), ) @@ -1449,7 +1460,7 @@ def test_forwards_profile_to_discovery(self, monkeypatch): monkeypatch.setattr(mcp, "ensure_databricks_auth", lambda workspace, profile=None: None) monkeypatch.setattr(mcp, "available_mcp_clients", lambda: ["claude"]) - def fake_services(workspace, profile=None): + def fake_services(workspace, profile=None, **kwargs): seen_profiles["mcp-services"] = profile return [] @@ -3820,7 +3831,9 @@ def test_no_source_prompt_opens_picker_directly(self, monkeypatch): monkeypatch.setattr(mcp.shutil, "which", lambda binary: f"/usr/bin/{binary}") monkeypatch.setattr(mcp, "ensure_databricks_auth", lambda workspace, profile=None: None) monkeypatch.setattr(mcp, "available_mcp_clients", lambda: ["claude"]) - monkeypatch.setattr(mcp, "discover_mcp_service_names", lambda workspace, profile=None: []) + monkeypatch.setattr( + mcp, "discover_mcp_service_names", lambda workspace, profile=None, **kwargs: [] + ) monkeypatch.setattr( mcp, "discover_all_mcp_service_names", @@ -3859,15 +3872,14 @@ def test_enumeration_merges_state_and_managed_files_scoped_by_agent(self, monkey assert mcp.managed_mcp_server_names(state) == {"from-state", "from-file", "codex-only"} @pytest.mark.parametrize("additive", [True, False]) - def test_managed_service_is_hidden_from_the_picker(self, additive): - # A managed server is never ug's to add or remove here (it lives in the OS-managed file), - # so it's hidden in both modes rather than shown as an inert row. - assert ( - mcp._mcp_service_choice( - "system.ai.github", {"system-ai-github"}, additive, {"system-ai-github"} - ) - is None + def test_managed_service_is_read_only_in_the_picker(self, additive): + # Managed services remain inspectable but cannot be added or removed. + choice = mcp._mcp_service_choice( + "system.ai.github", {"system-ai-github"}, additive, {"system-ai-github"} ) + assert choice.disabled == "Managed by workspace" + assert choice.checked is False + assert choice.status == "✓ Managed" def test_unmanaged_service_is_still_offered_as_an_add(self): choice = mcp._mcp_service_choice("system.ai.other", set(), True, {"system-ai-github"}) @@ -3875,9 +3887,8 @@ def test_unmanaged_service_is_still_offered_as_an_add(self): assert choice.disabled is None assert choice.value.startswith(mcp.MCP_ADD_PREFIX) - def test_add_picker_shows_only_addable_services(self): - # Under `ug mcp add`, the picker lists only what you can add: a managed service and one - # ug already configured are both hidden; an unconfigured one is offered. + def test_add_picker_shows_status_but_only_unconfigured_services_are_addable(self): + # Status is visible for every service; configured and managed rows are read-only. choices = mcp.build_mcp_picker_choices( [], [], @@ -3893,12 +3904,16 @@ def test_add_picker_shows_only_addable_services(self): ) titles = [c.title for c in choices] assert titles == [ - "MCP: system.ai.web_search" - ] # github (managed) + slack (configured) hidden + "system.ai.github", + "system.ai.slack", + "system.ai.web_search", + ] + assert [c.status for c in choices] == ["✓ Managed", "✓ Configured", "✗ Not configured"] + assert [bool(c.disabled) for c in choices] == [True, True, False] + assert all(not c.checked for c in choices) - def test_replace_picker_hides_managed_but_keeps_own_as_toggle(self): - # Replace mode still lets you remove your own servers (pre-checked toggle), but a managed - # one stays hidden since ug can't remove it here either. + def test_replace_picker_keeps_managed_read_only_and_own_as_toggle(self): + # Replace mode keeps managed rows read-only and ug's own rows removable. choices = mcp.build_mcp_picker_choices( [], [], @@ -3909,8 +3924,9 @@ def test_replace_picker_hides_managed_but_keeps_own_as_toggle(self): managed_names={"system-ai-github"}, ) by_title = {c.title: c for c in choices} - assert "MCP: system.ai.github" not in by_title # managed: hidden - assert by_title["MCP: system.ai.slack"].checked is True # own: removable toggle + assert by_title["system.ai.github"].disabled == "Managed by workspace" + assert by_title["system.ai.github"].checked is False + assert by_title["system.ai.slack"].checked is True # own: removable toggle def test_drop_managed_servers_reports_what_it_skipped(self, capsys): servers = [ diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index 51b4da4a7..7e0a13a6b 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -1000,18 +1000,21 @@ def ask(self): class TestSkillDownloadPicker: - def test_choice_value_is_fqn_flags_on_disk_and_carries_description(self, tmp_path): + def test_choice_value_is_fqn_and_unattributed_bundle_is_not_installed(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) fresh = sd._skill_download_choice(ref("triage", description="Routes tickets."), roots) assert fresh.value == "main.default.triage" - assert "(on disk)" not in fresh.title - assert fresh.description == "triage: Routes tickets." + assert fresh.status == "✗ Not installed" + assert "Full name: main.default.triage" in fresh.description + assert "Bundle: triage" in fresh.description + assert "Description: Routes tickets." in fresh.description write_skill(roots, ref("triage"), {"SKILL.md": b"x"}) existing = sd._skill_download_choice(ref("triage"), roots) assert existing.value == "main.default.triage" - assert "(on disk)" in existing.title + assert existing.status == "✗ Not installed" + assert "Local bundle: Already on disk" in existing.description def test_choice_description_labels_by_bundle_name_not_securable(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) @@ -1019,12 +1022,15 @@ def test_choice_description_labels_by_bundle_name_not_securable(self, tmp_path): choice = sd._skill_download_choice(diverging, roots) - assert choice.description == "task-triage: Ranks work." + assert "Bundle: task-triage" in choice.description + assert "Description: Ranks work." in choice.description - def test_choice_without_description_has_no_footer_text(self, tmp_path): + def test_choice_without_description_still_shows_metadata(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - assert sd._skill_download_choice(ref("triage"), roots).description is None + details = sd._skill_download_choice(ref("triage"), roots).description + assert "No description provided" in details + assert "Created by: Not available" in details def test_background_loader_streams_the_walk_in_as_choices(self, tmp_path, monkeypatch): roots = skill_dir_roots(str(tmp_path))