From 4a8eb554934a84ddbbc0e5e641ba854a0f5c894d Mon Sep 17 00:00:00 2001 From: Xi Lyu Date: Mon, 26 Oct 2026 12:59:56 +0000 Subject: [PATCH 01/11] copilot: register ug MCP servers in Copilot's user config ug now writes Copilot's MCP servers into Copilot's own user config, ~/.copilot/mcp-config.json (or $COPILOT_HOME/mcp-config.json), the way it writes Claude Code's ~/.claude.json and Codex's ~/.codex/config.toml. The ug-owned side file loaded only through --additional-mcp-config goes away: - Copilot's own `copilot mcp list` and plain `copilot` see ug's servers, and `ug mcp list` reads Copilot's status from `copilot mcp list --json` instead of reporting every server as missing. - Revert removes ug's servers one by one, like the other agents, instead of restoring a side-file backup that could already hold a ug server. - A same-name entry is replaced in place, as for Claude Code and Codex. - Servers in the old side file move into the user config on the next ug write or `ug copilot` launch; revert deletes the side file. - A user config that isn't valid JSON is left alone with an error. - `ug copilot` passes --allow-all-mcp-server-instructions: offline mode empties Copilot's instruction allowlist, so the gateway's servers lost their instructions. Tests: unit coverage for the writers, migration, COPILOT_HOME, revert and the JSON listing; a Copilot MCP integration journey (add, Copilot's listing, a headless tool call, revert); Copilot cases in the managed MCP and skills integration tests with their fixtures and a non-blocking managed Copilot lane. tests/AGENTS.md now admits Copilot MCP/skills journeys matching the Claude/Codex ones. Co-authored-by: Isaac --- .github/workflows/integration.yml | 57 +++++++++ README.md | 2 +- src/ucode/agents/copilot.py | 120 +++++++++++++----- src/ucode/mcp.py | 72 ++++++----- tests/AGENTS.md | 7 +- tests/README.md | 2 + tests/conftest.py | 12 ++ .../fixtures/managed_config/copilot_mcp.json | 24 ++++ .../copilot_skills_by_name.json | 24 ++++ tests/integration/README.md | 21 ++- .../test_ug_configure_managed_mcp.py | 28 +++- .../test_ug_configure_managed_skills.py | 27 +++- tests/integration/test_ug_copilot_mcp.py | 94 ++++++++++++++ tests/integration/utils/agents/copilot.py | 32 ++++- tests/integration/utils/evidence.py | 9 ++ tests/test_agent_copilot.py | 71 +++++++++-- tests/test_integration_contract.py | 1 + tests/test_mcp.py | 65 +++++++--- 18 files changed, 557 insertions(+), 111 deletions(-) create mode 100644 tests/fixtures/managed_config/copilot_mcp.json create mode 100644 tests/fixtures/managed_config/copilot_skills_by_name.json create mode 100644 tests/integration/test_ug_copilot_mcp.py diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index b5911fe56..77d02654b 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -478,6 +478,63 @@ jobs: ${{ runner.temp }}/ug-integration/artifacts/ ${{ runner.temp }}/ug-integration/wheels/ + # Non-blocking like the Copilot lane: continue-on-error and absent from the cujs `needs`. + # Runs the Copilot managed_fixture cases on the managed e2e workspace with injected configs that + # enable Copilot (a service principal authenticates it, like the `managed` lane). + managed-copilot: + name: Managed config · Copilot (non-blocking) + if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }} + continue-on-error: true + runs-on: ubuntu-22.04 + timeout-minutes: 30 + env: + UCODE_TEST_WORKSPACE: ${{ secrets.E2E_ADMIN_WORKSPACE }} + DATABRICKS_CLIENT_ID: ${{ secrets.E2E_ADMIN_SP_CLIENT_ID }} + DATABRICKS_CLIENT_SECRET: ${{ secrets.E2E_ADMIN_SP_CLIENT_SECRET }} + DEPENDENCY: ${{ inputs.dependency }} + TEST_MARKER: managed_fixture and copilot + ARTIFACT_NAME: integration-managed-copilot + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + ref: ${{ inputs.ref || github.sha }} + persist-credentials: false + fetch-depth: 0 + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 + with: + node-version: 22.19.0 + - uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0 + with: + version: 0.9.8 + enable-cache: ${{ inputs.ref == '' }} + - uses: databricks/setup-cli@bdb89f81c11a5bd647fd55b585b7c396ec68a25a # v1.0.0 + with: + version: 1.17.0 + - name: Run the Copilot managed cases against the managed workspace + shell: bash + run: | + args=(--copilot-version "$COPILOT_VERSION") + if [[ -n "$DEPENDENCY" ]]; then + args+=(--dependency "$DEPENDENCY") + fi + uv run --no-project --python 3.12 python scripts/run_integration.py \ + --python 3.12 --ug-version "$UG_VERSION" --entry-point "$ENTRY_POINT" \ + --default-index "$PACKAGE_INDEX" --output "$RUNNER_TEMP/ug-integration" \ + "${args[@]}" -- -m "$TEST_MARKER" + - name: Upload managed Copilot evidence + if: ${{ !cancelled() }} + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: ${{ env.ARTIFACT_NAME }} + include-hidden-files: true + path: | + ${{ runner.temp }}/ug-integration/*.json + ${{ runner.temp }}/ug-integration/*.txt + ${{ runner.temp }}/ug-integration/*.xml + ${{ runner.temp }}/ug-integration/*.log + ${{ runner.temp }}/ug-integration/artifacts/ + ${{ runner.temp }}/ug-integration/wheels/ + # managed and dedicated-cuj stay Linux-only: ug has no Windows managed-settings # writer, and the CUJ suite drives agents through a POSIX PTY. managed: diff --git a/README.md b/README.md index f3eb06598..7cf0f1f52 100644 --- a/README.md +++ b/README.md @@ -508,7 +508,7 @@ preferences are ignored and their files are left untouched. See the bundled | Claude Code | `~/.claude/ucode-settings.json`, `~/.claude.json`, `/etc/claude-code/managed-settings.json` (Linux), `/Library/Application Support/ClaudeCode/managed-settings.json` (macOS), `C:\Program Files\ClaudeCode\managed-settings.json` (Windows, written only from an elevated terminal) | | Gemini CLI | `~/.gemini/ucode.env`, `~/.ucode/.gemini-home/.gemini/settings.json` | | OpenCode | `~/.ucode/opencode-xdg/opencode/opencode.json`, `~/.ucode/opencode-xdg/opencode/plugin/ucode-auth.js` | -| GitHub Copilot CLI | `~/.copilot/ucode.env`, `~/.copilot/ucode-mcp-config.json` | +| GitHub Copilot CLI | `~/.copilot/ucode.env`, `~/.copilot/mcp-config.json` (or `$COPILOT_HOME/mcp-config.json`) | | Pi | `~/.ucode/pi-home/.pi/agent/models.json`, `~/.ucode/pi-home/.pi/agent/settings.json` | | Antigravity CLI | `~/.gemini/config/skills.json` (adds `~/.agents/skills`), `~/.gemini/config/mcp_config.json` (ug's MCP servers only); the gateway is set per launch through `AGY_LLM_GATEWAY_*` variables | | Cursor Agent | `~/.cursor/mcp.json` | diff --git a/src/ucode/agents/copilot.py b/src/ucode/agents/copilot.py index ed979b368..34f8c6139 100644 --- a/src/ucode/agents/copilot.py +++ b/src/ucode/agents/copilot.py @@ -41,8 +41,8 @@ from ucode.config_io import ( APP_DIR, ToolSpec, - apply_json_mcp_diff, backup_existing_file, + is_dry_run, parse_dotenv, read_json_safe, write_dotenv, @@ -88,9 +88,11 @@ COPILOT_CONFIG_DIR = Path.home() / ".copilot" COPILOT_ENV_PATH = COPILOT_CONFIG_DIR / "ucode.env" -COPILOT_MCP_CONFIG_PATH = COPILOT_CONFIG_DIR / "ucode-mcp-config.json" +COPILOT_MCP_CONFIG_PATH = COPILOT_CONFIG_DIR / "mcp-config.json" COPILOT_BACKUP_PATH = APP_DIR / "copilot-ucode-env.backup" -COPILOT_MCP_BACKUP_PATH = APP_DIR / "copilot-ucode-mcp-config.backup.json" +# Older ug versions kept their servers in a side file loaded with `--additional-mcp-config`. +LEGACY_MCP_CONFIG_PATH = COPILOT_CONFIG_DIR / "ucode-mcp-config.json" +LEGACY_MCP_BACKUP_PATH = APP_DIR / "copilot-ucode-mcp-config.backup.json" COPILOT_PROXY_LOG_PATH = APP_DIR / "logs" / "copilot-proxy.log" _REGISTRY_PREFIX = "ug-providers-" _REGISTRY_LOCK_NAME = "ug-providers.lock" @@ -800,35 +802,88 @@ def build_mcp_server_entry(argv: list[str]) -> dict: } -def write_mcp_server_config(name: str, argv: list[str]) -> bool: - backup_existing_file(COPILOT_MCP_CONFIG_PATH, COPILOT_MCP_BACKUP_PATH) - existing = read_json_safe(COPILOT_MCP_CONFIG_PATH) - mcp_servers = existing.get("mcpServers") - if not isinstance(mcp_servers, dict): - mcp_servers = {} - removed = name in mcp_servers - mcp_servers[name] = build_mcp_server_entry(argv) - existing["mcpServers"] = mcp_servers - write_json_file(COPILOT_MCP_CONFIG_PATH, existing) +def copilot_mcp_config_path() -> Path: + """The user config Copilot reads ``mcpServers`` from (and ``copilot mcp add`` writes): + ``$COPILOT_HOME/mcp-config.json`` when that env var is set, else ``~/.copilot/mcp-config.json``.""" + copilot_home = os.environ.get("COPILOT_HOME") + return Path(copilot_home) / "mcp-config.json" if copilot_home else COPILOT_MCP_CONFIG_PATH + + +def _read_mcp_config_for_rewrite(path: Path) -> dict: + """``path`` parsed for a rewrite (``{}`` when absent). Raises rather than return ``{}`` for a + file that isn't a JSON object, since rewriting it would drop the developer's own servers.""" + if not path.exists(): + return {} + try: + data = json.loads(path.read_text(encoding="utf-8")) + except (OSError, UnicodeError, json.JSONDecodeError): + data = None + if not isinstance(data, dict): + raise RuntimeError( + f"Can't update {path}: it isn't a valid JSON object. Fix it, then retry." + ) + return data + + +def remove_legacy_mcp_config() -> bool: + """Delete the legacy side file and its backup (on revert: everything in them is ug's).""" + existed = LEGACY_MCP_CONFIG_PATH.exists() or LEGACY_MCP_BACKUP_PATH.exists() + if is_dry_run(): + return existed + LEGACY_MCP_CONFIG_PATH.unlink(missing_ok=True) + LEGACY_MCP_BACKUP_PATH.unlink(missing_ok=True) + return existed + + +def _take_legacy_mcp_servers() -> dict[str, dict]: + """The servers in the legacy side file, deleting it and its backup. Every entry there was + written by ug, so it moves into the user config as-is.""" + servers = read_json_safe(LEGACY_MCP_CONFIG_PATH).get("mcpServers") + remove_legacy_mcp_config() + if not isinstance(servers, dict): + return {} + return {name: entry for name, entry in servers.items() if isinstance(entry, dict)} + + +def _apply_mcp_diff(add: dict[str, dict], remove: set[str]) -> set[str]: + path = copilot_mcp_config_path() + config = _read_mcp_config_for_rewrite(path) + servers = config.get("mcpServers") + if not isinstance(servers, dict): + servers = {} + legacy = _take_legacy_mcp_servers() + removed = {name for name in remove if name in servers or name in legacy} + if not add and not removed and not legacy: + return removed + servers.update({name: entry for name, entry in legacy.items() if name not in remove}) + for name in remove: + servers.pop(name, None) + servers.update(add) + config["mcpServers"] = servers + write_json_file(path, config) return removed +def migrate_legacy_mcp_config() -> None: + """Move the servers in the legacy side file into the user config, once.""" + if LEGACY_MCP_CONFIG_PATH.exists() or LEGACY_MCP_BACKUP_PATH.exists(): + _apply_mcp_diff({}, set()) + + +def write_mcp_server_config(name: str, argv: list[str]) -> bool: + """Add (or replace) a stdio (`ug mcp-proxy`) MCP server in Copilot's user config. Returns + True when an entry of that name was replaced.""" + return bool(_apply_mcp_diff({name: build_mcp_server_entry(argv)}, {name})) + + def remove_mcp_server_config(name: str) -> bool: - existing = read_json_safe(COPILOT_MCP_CONFIG_PATH) - mcp_servers = existing.get("mcpServers") - if not isinstance(mcp_servers, dict) or name not in mcp_servers: - return False - mcp_servers.pop(name) - existing["mcpServers"] = mcp_servers - write_json_file(COPILOT_MCP_CONFIG_PATH, existing) - return True + return bool(_apply_mcp_diff({}, {name})) def write_user_mcp_servers(add: dict[str, dict], remove: set[str]) -> set[str]: - """Apply ``add``/``remove`` to Copilot's `mcpServers` in a single read-modify-write. Returns the names actually removed.""" - return apply_json_mcp_diff( - COPILOT_MCP_CONFIG_PATH, "mcpServers", add, remove, backup_path=COPILOT_MCP_BACKUP_PATH - ) + """Apply ``add``/``remove`` to Copilot's user-config ``mcpServers`` in a single + read-modify-write. Returns the names actually removed.""" + return _apply_mcp_diff(add, remove) def scrub_stored_credentials() -> None: @@ -951,6 +1006,10 @@ def launch(state: dict, tool_args: list[str], *, options: LaunchOptions) -> None managed_list=not (provider_service or model_location), ) scrub_stored_credentials() + try: + migrate_legacy_mcp_config() + except RuntimeError as exc: + print_warning(f"ug's MCP servers aren't available in this session: {exc}") # Only Copilot launches need the proxy (and its `httpx` import). from ucode import gateway_proxy @@ -986,7 +1045,7 @@ def launch(state: dict, tool_args: list[str], *, options: LaunchOptions) -> None with session_registry(config) as registry_path: env = build_runtime_env(registry_path, start_model) proc = subprocess_cross_os.popen( - [SPEC["binary"], *mcp_config_args(), *tool_args], env=env + [SPEC["binary"], *MCP_LAUNCH_ARGS, *tool_args], env=env ) returncode = _wait_for_exit(proc) finally: @@ -1000,14 +1059,13 @@ def launch(state: dict, tool_args: list[str], *, options: LaunchOptions) -> None def validate_cmd(binary: str) -> list[str]: return [ binary, - *mcp_config_args(), + *MCP_LAUNCH_ARGS, "--prompt", "say hi in 5 words or less", "--allow-all-tools", ] -def mcp_config_args() -> list[str]: - if not COPILOT_MCP_CONFIG_PATH.exists(): - return [] - return ["--additional-mcp-config", f"@{COPILOT_MCP_CONFIG_PATH}"] +# Offline mode empties Copilot's allowlist of servers whose `instructions` reach the model, so +# without this the gateway's servers lose theirs. +MCP_LAUNCH_ARGS = ("--allow-all-mcp-server-instructions",) diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index d441c345d..f9d342d1a 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json import os import re import shutil @@ -16,7 +17,7 @@ from rich.table import Table from ucode.agents import antigravity, claude, codex, copilot, cursor, gemini, kilo, opencode -from ucode.config_io import read_json_safe, read_toml_safe, restore_file +from ucode.config_io import read_json_safe, read_toml_safe from ucode.constants import MCP_CLEANUP_SCOPES, MCP_USER_SCOPE from ucode.databricks import ( AIGW_MCP_SERVICES_SEGMENT, @@ -110,7 +111,7 @@ "copilot": { "binary": "copilot", "display": "GitHub Copilot CLI", - "list_command": "copilot mcp list", + "list_command": "copilot mcp list --json", }, "cursor": { "binary": "cursor-agent", @@ -456,6 +457,8 @@ def revert_mcp_configs(state: dict) -> dict[str, bool]: all_servers = list(state.get("mcp_servers") or []) + list( state.get("managed_mcp_servers") or [] ) + if copilot.remove_legacy_mcp_config(): + results["copilot"] = True for server in all_servers: name = server.get("name") if not isinstance(name, str) or not name: @@ -466,16 +469,6 @@ def revert_mcp_configs(state: dict) -> dict[str, bool]: removed_scopes = remove_client_mcp_server(client, name) results[client] = bool(removed_scopes) or results.get(client, False) - # OpenCode MCP entries live in the normal OpenCode config and are restored - # by the main agent config revert. Copilot stores MCP servers separately, - # so restore its original MCP file after removing per-server entries above. - results["copilot"] = restore_file( - copilot.COPILOT_MCP_CONFIG_PATH, - copilot.COPILOT_MCP_BACKUP_PATH, - any( - "copilot" in (server.get("clients") or []) for server in state.get("mcp_servers") or [] - ), - ) or results.get("copilot", False) return results @@ -2367,10 +2360,11 @@ def remove_mcp_command(agents: set[str] | None = None) -> int: # `ug mcp list`: configured MCP servers + their live per-agent connection status # --------------------------------------------------------------------------- -# Per-(server, agent) states surfaced by `ug mcp list`. Agent listings differ: some -# health-check servers, Codex reports enabled/disabled, and Copilot's ug-added entries -# are visible only in the additional config file. NOT_REGISTERED means the agent is -# installed but neither its listing nor ug's additional config contains the server. +# Per-(server, agent) live states surfaced by `ug mcp list`. Every agent's `mcp list` +# health-checks its servers and reports connected/failed, except Codex, Antigravity and +# Copilot, whose listings report only whether an entry is enabled/disabled (no probe). +# NOT_REGISTERED means the agent is installed but doesn't list the server; a server maps to +# None (rendered "agent not installed") when the agent binary isn't installed at all. LIVE_CONNECTED = "connected" LIVE_FAILED = "failed" LIVE_ENABLED = "enabled" @@ -2510,6 +2504,24 @@ def _parse_tree_mcp_list(output: str) -> dict[str, str]: return statuses +def _parse_copilot_mcp_list(output: str) -> dict[str, str]: + """Parse `copilot mcp list --json` into ``{server_name: state}``. + + Like Codex, Copilot reports config state (``enabled``), not a health probe. Output that isn't + the expected JSON object contributes nothing.""" + try: + servers = json.loads(output[output.find("{") :]).get("mcpServers") + except (json.JSONDecodeError, AttributeError): + return {} + if not isinstance(servers, dict): + return {} + return { + name: LIVE_ENABLED if entry.get("enabled", True) else LIVE_DISABLED + for name, entry in servers.items() + if isinstance(entry, dict) + } + + def parse_mcp_list_output(client: str, output: str) -> dict[str, str]: """Parse an agent's `mcp list` output into ``{server_name: live-state}`` (best-effort). @@ -2521,6 +2533,8 @@ def parse_mcp_list_output(client: str, output: str) -> dict[str, str]: if client in ("codex", "antigravity"): # Both print a columnar table with an enabled/disabled state column. parsed = _parse_codex_mcp_list(output) + elif client == "copilot": + parsed = _parse_copilot_mcp_list(output) elif client in ("kilo", "opencode"): parsed = _parse_tree_mcp_list(output) else: @@ -2564,18 +2578,11 @@ def _run_mcp_list(client: str) -> str | None: def query_live_mcp_status(client: str) -> dict[str, str]: - """MCP ``{server_name: state}`` for one installed agent, including ug's Copilot config.""" + """Live ``{server_name: state}`` for one installed agent (empty if its listing can't be read).""" output = _run_mcp_list(client) - statuses = parse_mcp_list_output(client, output) if output is not None else {} - if client == "copilot": - # Copilot reads ug's additional MCP config only on an agent launch. Its bare `mcp list` - # omits these servers, so report their configured state without claiming a live probe. - servers = read_json_safe(copilot.COPILOT_MCP_CONFIG_PATH).get("mcpServers") - if isinstance(servers, dict): - for name, entry in servers.items(): - if isinstance(name, str) and isinstance(entry, dict): - statuses.setdefault(name, LIVE_ENABLED) - return statuses + if output is None: + return {} + return parse_mcp_list_output(client, output) def _query_live_statuses(clients: list[str]) -> dict[str, dict[str, str]]: @@ -2799,10 +2806,13 @@ def list_mcp_command(agents: set[str] | None = None) -> int: print_note(f"Other MCP servers not configured by ug — {other_summary}.") live_note = "Status is from each agent's `mcp list`" - if "codex" in probe_clients: - live_note += "; Codex reports enabled/disabled" - if "copilot" in probe_clients: - live_note += "; ug-added Copilot servers report configured/enabled, not live health" + # Only mention the enabled/disabled caveat for the agents actually in the reported set. + config_state_clients = [ + MCP_CLIENTS[client]["display"] for client in ("codex", "copilot") if client in probe_clients + ] + if config_state_clients: + verb = "reports" if len(config_state_clients) == 1 else "report" + live_note += f"; {' and '.join(config_state_clients)} {verb} enabled/disabled" print_note(f"{live_note}.") # A `needs sign-in` server is registered fine but needs the one-time per-user connection login; # point the developer at it rather than leaving an opaque status. diff --git a/tests/AGENTS.md b/tests/AGENTS.md index 0093c805f..e2fbe07da 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -93,9 +93,10 @@ exercise. and run documentation at the suite root. - Current scope is basic Claude/Codex configuration, routing, script usage, command forwarding, and configure/revert CUJs, plus Copilot CLI headless - model tasks and one TUI model-switch journey (non-blocking lane), plus Antigravity CLI - configure, TUI, headless, External Models, MCP-tool, skill-directory, and revert journeys - (blocking Linux lane). Do not add other MCP/skills functionality, + model tasks, one TUI model-switch journey, and Copilot MCP/skills journeys matching the + Claude/Codex ones (non-blocking lanes), plus Antigravity CLI configure, TUI, headless, + External Models, MCP-tool, skill-directory, and revert journeys (blocking Linux lane). + Do not add other MCP/skills functionality, tracing, or the broad configure-option matrix without a new scope request. Existing `mcp --help` checks cover dispatch only. diff --git a/tests/README.md b/tests/README.md index d0559198f..d7438fc49 100644 --- a/tests/README.md +++ b/tests/README.md @@ -678,6 +678,8 @@ integration utilities; only CUJ-specific evidence correlation stays in a test fi | `test_ug_copilot_tui_switches_from_claude_to_gpt_with_model_command` | TUI: answer folder trust, Claude task, `/model dbx-responses/`, GPT task, `/exit` | Both answers sent as the Claude then GPT model; the session records the switch; normal exit; no session registry left (non-blocking CI lane) | | `test_ug_copilot_refreshes_a_revoked_token_mid_session`, `test_ug_copilot_reports_expired_sign_in` | Sign in through `DATABRICKS_BEARER_COMMAND` minting short-lived PATs; mid-task the agent revokes them (or also signs out) | Revoked: a token is minted after the revocation and the task completes without restarting Copilot. Signed out: output shows "Databricks sign-in expired" with `databricks auth login`; the failure is in `~/.ucode/logs/copilot-proxy.log`, not the terminal (non-blocking CI lane) | | `test_ug_configure_copilot_repeat_and_revert` | Configure Copilot twice over a user-owned `~/.copilot/mcp-config.json`, run a GPT task, revert twice | Task completes; no file holds the bearer; revert removes `~/.copilot/ucode.env`, leaves the user file byte-identical and reports Not Configured. (non-blocking CI lane) | +| `test_ug_mcp_add_copilot_registers_server_in_copilot_config` | Over a developer-owned, disabled server in `~/.copilot/mcp-config.json`, configure Copilot, `ug mcp add --agents copilot --names system.ai.web_search`, run a headless task that calls the tool, then revert | `copilot mcp list --json` lists ug's server from the user config, enabled, next to the developer's still-disabled one; `ug mcp list --agents copilot` reports it enabled; Copilot's session events record a successful call loaded from the user config; revert leaves only the developer's server and no legacy `ucode-mcp-config.json` (non-blocking CI lane) | +| `test_managed_fixture_copilot_mcp_registered_in_user_config`, `test_managed_fixture_copilot_skills_lists_downloaded_skill` | Non-interactive configure with an injected config enabling Copilot with a managed MCP server or a managed skill | `copilot mcp list --json` lists the managed server from the user config as the ug mcp-proxy command; `copilot skill list --json` lists the downloaded skill as an enabled personal skill (non-blocking managed Copilot lane) | | `test_ug_copilot_upgrade_scrubs_token_saved_by_older_ug` | Configure Copilot with ug v0.2.0 (pinned commit; saves the bearer in `~/.copilot/ucode.env`), then configure with this ug, run a GPT task, revert | v0.2.0 left the bearer; after this ug's configure no file holds it; task completes; revert leaves no token and no `ucode.env` (non-blocking CI lane) | | `test_ug_opencode_headless_prompt_argument` | Run OpenCode from a script (`run --format json --auto`) with an argument prompt | Completed Read tool call; final text answer contains the file value; exit zero (non-blocking CI lane) | | `test_ug_agents_self_managed_opencode_journey` | Under the injected `managed_workspace_default` config (enables Claude/Codex, not OpenCode): bare configure, `ug agents list`, refused OpenCode launch, `ug agents add opencode`, real headless task, `ug agents remove opencode` (marker `managed_fixture and opencode`, non-blocking CI lane) | OpenCode absent from the list and launch fails with "doesn't enable OpenCode" / `ug agents add opencode`; after add it is listed self-managed and the headless Read task returns the fixture value; after remove it is hidden and refused again | diff --git a/tests/conftest.py b/tests/conftest.py index 1f3470782..a7017fab2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -40,6 +40,7 @@ def _isolate_ucode_state(tmp_path, monkeypatch): from ucode.agents import antigravity as antigravity_mod from ucode.agents import claude as claude_mod from ucode.agents import codex as codex_mod + from ucode.agents import copilot as copilot_mod from ucode.agents import vscode as vscode_mod from ucode.agents.desktop import claude as desktop_mod @@ -115,6 +116,17 @@ def install_test_policy(path, desired_text, *, interactive=False, allow_sudo=Tru codex_mod, "CODEX_MODEL_CATALOG_PATH", state_dir / "codex-model-catalog.json" ) monkeypatch.setattr(codex_mod, "CODEX_CONFIG_PATH", tmp_path / ".codex" / "ucode.config.toml") + # MCP writes and revert edit Copilot's user config and delete the legacy side file. + monkeypatch.setattr( + copilot_mod, "COPILOT_MCP_CONFIG_PATH", tmp_path / ".copilot" / "mcp-config.json" + ) + monkeypatch.setattr( + copilot_mod, "LEGACY_MCP_CONFIG_PATH", tmp_path / ".copilot" / "ucode-mcp-config.json" + ) + monkeypatch.setattr( + copilot_mod, "LEGACY_MCP_BACKUP_PATH", state_dir / "copilot-ucode-mcp-config.backup.json" + ) + monkeypatch.delenv("COPILOT_HOME", raising=False) def reject_privileged_write(path, _desired_text): pytest.fail( diff --git a/tests/fixtures/managed_config/copilot_mcp.json b/tests/fixtures/managed_config/copilot_mcp.json new file mode 100644 index 000000000..8cec6a864 --- /dev/null +++ b/tests/fixtures/managed_config/copilot_mcp.json @@ -0,0 +1,24 @@ +{ + "spec_version": 1, + "default_agent": "CODING_AGENT_COPILOT", + "enabled_agents": [ + { + "agent": "CODING_AGENT_COPILOT", + "config": { + "models": { + "model_services": [ + "system.ai.claude-sonnet-4-6" + ] + }, + "default_models": { + "default_model": "system.ai.claude-sonnet-4-6" + } + } + } + ], + "mcp_servers": { + "names": [ + "system.ai.github" + ] + } +} diff --git a/tests/fixtures/managed_config/copilot_skills_by_name.json b/tests/fixtures/managed_config/copilot_skills_by_name.json new file mode 100644 index 000000000..0d6c2d02b --- /dev/null +++ b/tests/fixtures/managed_config/copilot_skills_by_name.json @@ -0,0 +1,24 @@ +{ + "spec_version": 1, + "default_agent": "CODING_AGENT_COPILOT", + "enabled_agents": [ + { + "agent": "CODING_AGENT_COPILOT", + "config": { + "models": { + "model_services": [ + "system.ai.claude-sonnet-4-6" + ] + }, + "default_models": { + "default_model": "system.ai.claude-sonnet-4-6" + } + } + } + ], + "skills": { + "names": [ + "main.default.forkable-meals" + ] + } +} diff --git a/tests/integration/README.md b/tests/integration/README.md index ff07fd949..f57d1fea5 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -338,6 +338,12 @@ model-discovery schema and completes a file task on its Claude-compatible model `test_ug_copilot_providers.py` runs Copilot through the MPS fixtures below: a file task on the Anthropic service's model, on the OpenAI, Azure OpenAI and Microsoft Foundry services' `gpt-5-nano`, and on a named model of the Bedrock allow-all service; launching on the relayed service fails before Copilot starts. +`test_ug_copilot_mcp.py` adds `system.ai.web_search` with `ug mcp add --agents copilot` over a +developer's own disabled server, checks Copilot's `copilot mcp list --json` and `ug mcp list`, runs +a headless task whose tool call Copilot's session events record as successful, and reverts. The +Copilot cases in `test_ug_configure_managed_mcp.py` and `test_ug_configure_managed_skills.py` check +that a managed server and a managed skill reach Copilot (`copilot mcp list --json`, `copilot skill +list --json`). Antigravity CLI has five live headless journeys in `test_ug_antigravity_headless.py` and three in `test_ug_configure_antigravity.py`. Headless runs use `agy -p --output-format stream-json`; the @@ -665,6 +671,7 @@ test_ug_copilot_lifecycle.py # Copilot refresh after revocation, sign test_ug_copilot_models.py # Copilot model registry: Claude then GPT, unknown model, TUI /model switch test_ug_copilot_providers.py # Copilot via Model Provider Services: Anthropic, OpenAI, Azure OpenAI, Foundry, Bedrock allow-all; relayed refused test_ug_copilot_model_location.py # Copilot on a Unity Catalog model location: fresh --model-location file task +test_ug_copilot_mcp.py # Copilot MCP: ug mcp add, tool call, ug mcp list, revert keeps the user's server test_ug_antigravity_headless.py # Antigravity via ug's proxy: file task, --model (ug agy), Gemini MPS targets, MCP SQL tool, skill dir test_ug_configure_antigravity.py # Antigravity: configure picker + TUI task + reopen; External Models picker; revert keeps user files test_ug_codex_tracing.py # Codex OTLP export reaches the configured trace table @@ -683,8 +690,8 @@ test_ug_claude_model_discovery.py # unmanaged scenarios 9, 11, 13 test_ug_codex_model_discovery.py # unmanaged scenarios 10, 12, 14 test_ug_configure_managed.py # stubbed static model list/catalog pointer, no agent selector, saved-config replay test_ug_configure_managed_models.py # stubbed model sources, smart-routing banner, Codex fallback metadata -test_ug_configure_managed_mcp.py # stubbed managed MCP list -test_ug_configure_managed_skills.py # stubbed managed skills: download, coexist, reconcile away +test_ug_configure_managed_mcp.py # stubbed managed MCP list (Claude, Codex, Copilot) +test_ug_configure_managed_skills.py # stubbed managed skills: download, coexist, reconcile away (Claude, Codex, Copilot) test_ug_configure_managed_lifecycle.py # none -> A -> B -> MPS -> none: reconcile, clear on MPS/no-config test_installation.py # fresh installed package utils/ # process/terminal/evidence helpers and Docker files @@ -875,9 +882,9 @@ cases instead complete real file tasks through `--workspace` and `--model-locati journeys check setup and launch with existing services, without inference. There are **88 live cases** (including 12 marked TUI journeys) and **7 installation -checks** with Claude and Codex; selecting OpenCode adds one live headless case and two `managed_fixture` cases; selecting Copilot adds seventeen live cases; selecting Antigravity adds eight live cases (one TUI). One +checks** with Claude and Codex; selecting OpenCode adds one live headless case and two `managed_fixture` cases; selecting Copilot adds eighteen live cases and two `managed_fixture` cases; selecting Antigravity adds eight live cases (one TUI). One **`workspace_switch` case** uses two real workspaces and checks skills MCP cleanup and a completed -Claude task. A further **44 `managed_fixture` cases** (twelve of them also `live`) run on the +Claude task. A further **46 `managed_fixture` cases** (twelve of them also `live`) run on the managed workspace with a checked-in JSON CodingAgentConfig from `tests/fixtures/managed_config/` injected through `UCODE_MANAGED_CONFIG_STUB`; there are no cases that read a published config. Twelve explicit configured/fresh Claude and Codex discovery and source-override journeys use the @@ -1101,8 +1108,10 @@ A second non-blocking job, **Managed OpenCode self-managed** (`managed_fixture a in `test_ug_agents_self_managed.py`), installs Claude, Codex and OpenCode and runs against the managed e2e workspace (`E2E_ADMIN_WORKSPACE`) with the injected `managed_workspace_default` config, which enables Claude/Codex but not OpenCode. It is likewise outside `cujs`. -A non-blocking **Copilot** job (`live and copilot`, sixteen cases) runs on Linux and Windows the -same way and is likewise outside `cujs` until it is stable. +A non-blocking **Copilot** job (`live and copilot`, seventeen cases) runs on Linux and Windows the +same way and is likewise outside `cujs` until it is stable. A non-blocking **Managed config · Copilot** +job (`managed_fixture and copilot`, two cases) installs only Copilot and runs on the managed e2e +workspace with injected configs that enable Copilot; it is likewise outside `cujs`. A blocking **Antigravity** job (`live and antigravity`, eight cases) runs on Linux only with the pinned `ANTIGRAVITY_BUILD`; it is part of the `cujs` gate. diff --git a/tests/integration/test_ug_configure_managed_mcp.py b/tests/integration/test_ug_configure_managed_mcp.py index bd158cae2..2c94f5019 100644 --- a/tests/integration/test_ug_configure_managed_mcp.py +++ b/tests/integration/test_ug_configure_managed_mcp.py @@ -10,9 +10,11 @@ the developer's own ~/.codex/config.toml servers. A non-interactive configure has no sudo reconcile, so it falls back to the user-scope registration, which the real /mcp view still lists. Claude's `managedMcpServers` path is covered by unit tests; its integration coverage needs the managed -workspace to publish the `claude-code` OAuth client, which E2E_ADMIN_WORKSPACE does not. +workspace to publish the `claude-code` OAuth client, which E2E_ADMIN_WORKSPACE does not. Copilot +has no OS-managed MCP file, so its managed servers land in its user config, ~/.copilot/mcp-config.json. """ +import json import tomllib import pytest @@ -114,3 +116,27 @@ def test_managed_fixture_codex_http_headers_in_managed_file(live_session, worksp f"Expected {managed_header_key!r} = {managed_header_value!r}, " f"got {headers[managed_header_key]!r}" ) + + +@pytest.mark.managed_fixture +@pytest.mark.copilot +def test_managed_fixture_copilot_mcp_registered_in_user_config(live_session, workspace): + """Scenario: a non-interactive configure with a managed MCP server enabled for Copilot. + + Expected: Copilot's own `copilot mcp list --json` lists the server from its user config, + enabled, as the ug mcp-proxy stdio command for the server's mcp-services URL. + """ + session = live_session + use_managed_config_fixture(session, "copilot_mcp") + result = session.run("configure", "--workspace", workspace, "--skip-upgrade", timeout=240) + assert "Select coding agents to configure:" not in result.stdout, result.stdout + + listing = session.run("mcp", "list", "--json", binary=session.which("copilot"), timeout=60) + servers = json.loads(listing.stdout)["mcpServers"] + entries = {name: entry for name, entry in servers.items() if "github" in name.lower()} + assert entries, servers + entry = next(iter(entries.values())) + assert entry["source"] == "user", entry + assert entry["enabled"] is True, entry + assert entry["command"].endswith("ug"), entry + assert "mcp-services" in entry["args"][entry["args"].index("--url") + 1], entry diff --git a/tests/integration/test_ug_configure_managed_skills.py b/tests/integration/test_ug_configure_managed_skills.py index 9a96eda57..f650c1e4d 100644 --- a/tests/integration/test_ug_configure_managed_skills.py +++ b/tests/integration/test_ug_configure_managed_skills.py @@ -3,12 +3,14 @@ The admin CodingAgentConfig is a checked-in JSON fixture injected via UCODE_MANAGED_CONFIG_STUB so the real /skills TUI can be driven against a managed skill; only the config INPUT is stubbed (auth, the skill download, the config writers, and the agent binary stay real). These assert what the agent presents, not the bundles on disk (that is unit tests' job). See -tests/AGENTS.md rule 4. Claude reads ~/.claude/skills and Codex reads the shared ~/.agents/skills, -both of which `ug configure` writes, so a managed skill reaches either agent. The reconcile +tests/AGENTS.md rule 4. Claude reads ~/.claude/skills and Codex and Copilot read the shared +~/.agents/skills, both of which `ug configure` writes, so a managed skill reaches each agent. The reconcile lifecycle test additionally checks the on-disk bundles, since removal-on-reconcile is a disk change that has no steady-state `/skills` signal. """ +import json + import pytest from utils.managed import use_managed_config_fixture from utils.terminal import AgentTerminal @@ -79,6 +81,27 @@ def test_managed_fixture_codex_skills_lists_downloaded_skill(live_session, works ) +@pytest.mark.managed_fixture +@pytest.mark.copilot +def test_managed_fixture_copilot_skills_lists_downloaded_skill(live_session, workspace): + """Scenario: configure Copilot under an injected config that names a managed skill, then list + Copilot's skills. + + Expected: Copilot's own `copilot skill list --json` lists the downloaded managed skill as an + enabled personal skill, read from the shared ~/.agents/skills dir `ug configure` writes. + """ + session = live_session + use_managed_config_fixture(session, "copilot_skills_by_name") + result = session.run("configure", "--workspace", workspace, "--skip-upgrade", timeout=240) + assert "Select coding agents to configure:" not in result.stdout, result.stdout + + listing = session.run("skill", "list", "--json", binary=session.which("copilot"), timeout=60) + skills = [skill for skill in json.loads(listing.stdout) if skill.get("name") == SKILL_NAME] + assert skills, listing.stdout + assert all(skill["enabled"] is True for skill in skills), skills + assert any(str(skill["source"]).startswith("personal") for skill in skills), skills + + @pytest.mark.managed_fixture @pytest.mark.claude def test_managed_skills_reconcile_lifecycle(live_session, workspace): diff --git a/tests/integration/test_ug_copilot_mcp.py b/tests/integration/test_ug_copilot_mcp.py new file mode 100644 index 000000000..3dd82656a --- /dev/null +++ b/tests/integration/test_ug_copilot_mcp.py @@ -0,0 +1,94 @@ +"""CUJ for Databricks MCP servers in GitHub Copilot CLI through installed ug.""" + +import json + +import pytest +from utils.constants import CLAUDE_TEST_MODEL, COPILOT_HEADLESS_FLAGS +from utils.evidence import copilot_mcp_tool_calls +from utils.terminal import TerminalProcess + +pytestmark = [pytest.mark.live, pytest.mark.copilot] + +MCP_SERVICE = "system.ai.web_search" +# The developer's own server, disabled in Copilot's settings so the session never starts it. +USER_SERVER = "developer-own" +USER_ENTRY = {"type": "local", "command": "developer-own-mcp", "args": [], "tools": ["*"]} + + +def _copilot_mcp_servers(session) -> dict: + """Copilot's own view of its configured MCP servers (`copilot mcp list --json`).""" + result = session.run("mcp", "list", "--json", binary=session.which("copilot"), timeout=60) + return json.loads(result.stdout)["mcpServers"] + + +def test_ug_mcp_add_copilot_registers_server_in_copilot_config(live_session, workspace): + """Scenario: with a disabled server of their own in Copilot's user config, the developer + configures Copilot, adds `system.ai.web_search` with `ug mcp add --agents copilot`, asks + `ug copilot` to use it in a headless task, then runs `ug revert`. + + Expected: Copilot's own `copilot mcp list --json` lists ug's server from its user config + next to the developer's still-disabled server; `ug mcp list --agents copilot` reports it + enabled; Copilot's session events show a successful call to the server's tool, loaded from + the user config; revert removes only ug's server, so Copilot lists just the developer's. + Limits: the service needs no connection sign-in, so the sign-in path is not covered here. + """ + session = live_session + # Ordinary pre-existing user settings, not manufactured gateway state. + user_config = session.home / ".copilot/mcp-config.json" + user_config.parent.mkdir(parents=True) + user_config.write_text(json.dumps({"mcpServers": {USER_SERVER: USER_ENTRY}}), encoding="utf-8") + user_settings = session.home / ".copilot/settings.json" + user_settings.write_text(json.dumps({"disabledMcpServers": [USER_SERVER]}), encoding="utf-8") + + session.run( + "configure", + "--agents", + "copilot", + "--workspace", + workspace, + "--skip-validate", + "--skip-upgrade", + "--disable-databricks-ai-tools", + ) + session.run( + "mcp", "add", "--agents", "copilot", "--workspace", workspace, "--names", MCP_SERVICE + ) + [server] = [ + entry["name"] + for entry in session.workspace_state()["mcp_servers"] + if MCP_SERVICE in str(entry.get("url")) + ] + + servers = _copilot_mcp_servers(session) + assert servers[server]["source"] == "user", servers + assert servers[server]["enabled"] is True, servers + assert servers[USER_SERVER]["enabled"] is False, servers + listing = session.run("mcp", "list", "--agents", "copilot").stdout + rows = [line for line in listing.splitlines() if server in line] + assert rows and all("enabled" in row for row in rows), listing + + session.record("copilot-model.json", {"model": CLAUDE_TEST_MODEL}) + session.run( + "copilot", + "--model", + CLAUDE_TEST_MODEL, + "-p", + f"Call the `{server}` web_search tool once to search for 'Databricks Unity Catalog'. " + "Then reply with the single word DONE.", + *COPILOT_HEADLESS_FLAGS, + timeout=240, + ) + calls = [call for call in copilot_mcp_tool_calls(session) if call["server"] == server] + assert calls, copilot_mcp_tool_calls(session) + assert all(call["source"] == "user" for call in calls), calls + assert any(call["succeeded"] for call in calls), calls + + with TerminalProcess(session, "ug", [str(session.binary), "revert"], "revert") as terminal: + terminal.finish() + assert set(_copilot_mcp_servers(session)) == {USER_SERVER} + assert json.loads(user_config.read_text(encoding="utf-8")) == { + "mcpServers": {USER_SERVER: USER_ENTRY} + } + settings = json.loads(user_settings.read_text(encoding="utf-8")) + assert settings["disabledMcpServers"] == [USER_SERVER], settings + assert not (session.home / ".copilot/ucode-mcp-config.json").exists() diff --git a/tests/integration/utils/agents/copilot.py b/tests/integration/utils/agents/copilot.py index 2ea73799d..4244a86df 100644 --- a/tests/integration/utils/agents/copilot.py +++ b/tests/integration/utils/agents/copilot.py @@ -2,7 +2,13 @@ from . import _evidence_id -__all__ = ["EVIDENCE_KIND", "assistant_answers", "completed_task_models", "selected_models"] +__all__ = [ + "EVIDENCE_KIND", + "assistant_answers", + "completed_task_models", + "mcp_tool_calls", + "selected_models", +] SESSION_DIRECTORY = ".copilot/session-state" EVIDENCE_KIND = "assistant message model" @@ -46,3 +52,27 @@ def selected_models(records: list[dict]) -> list[str]: elif record.get("type") == "session.model_change" and data.get("newModel"): refs.append(data["newModel"]) return refs + + +def mcp_tool_calls(records: list[dict]) -> list[dict]: + """Each MCP tool call: its server, the config source Copilot loaded it from, and whether + the call completed successfully.""" + succeeded = { + (record.get("data") or {}).get("toolCallId") + for record in records + if record.get("type") == "tool.execution_complete" + and (record.get("data") or {}).get("success") is True + } + calls = [] + for record in records: + data = record.get("data") or {} + if record.get("type") == "tool.execution_start" and data.get("mcpServerName"): + calls.append( + { + "server": data["mcpServerName"], + "tool": data.get("mcpToolName"), + "source": data.get("mcpConfigSource"), + "succeeded": data.get("toolCallId") in succeeded, + } + ) + return calls diff --git a/tests/integration/utils/evidence.py b/tests/integration/utils/evidence.py index 0a1798b8e..9fb855939 100644 --- a/tests/integration/utils/evidence.py +++ b/tests/integration/utils/evidence.py @@ -156,6 +156,15 @@ def copilot_selected_models(session) -> list[str]: ] +def copilot_mcp_tool_calls(session) -> list[dict]: + """Every MCP tool call recorded by Copilot sessions under this home.""" + return [ + call + for records in agent_sessions(session, "copilot").values() + for call in copilot.mcp_tool_calls(records) + ] + + def assert_no_session_registry_left(session) -> None: """`ug copilot` removes its per-session registry, which holds the proxy secret, on exit.""" leftovers = sorted(session.home.glob(".copilot/ug-providers-*")) diff --git a/tests/test_agent_copilot.py b/tests/test_agent_copilot.py index 2bebd3a3e..82f682cd5 100644 --- a/tests/test_agent_copilot.py +++ b/tests/test_agent_copilot.py @@ -830,9 +830,7 @@ def test_writes_mcp_server_without_clobbering_existing_config(self, tmp_path, mo monkeypatch.setattr(config_io_mod, "APP_DIR", tmp_path) config_file = tmp_path / "mcp-config.json" - backup_file = tmp_path / "copilot-mcp-backup.json" monkeypatch.setattr(cp_mod, "COPILOT_MCP_CONFIG_PATH", config_file) - monkeypatch.setattr(cp_mod, "COPILOT_MCP_BACKUP_PATH", backup_file) config_file.write_text( json.dumps( @@ -863,9 +861,7 @@ def test_reports_replaced_mcp_server(self, tmp_path, monkeypatch): monkeypatch.setattr(config_io_mod, "APP_DIR", tmp_path) config_file = tmp_path / "mcp-config.json" - backup_file = tmp_path / "copilot-mcp-backup.json" monkeypatch.setattr(cp_mod, "COPILOT_MCP_CONFIG_PATH", config_file) - monkeypatch.setattr(cp_mod, "COPILOT_MCP_BACKUP_PATH", backup_file) config_file.write_text( json.dumps({"mcpServers": {"github": {"old": True}}}), @@ -905,6 +901,51 @@ def test_removes_mcp_server_without_clobbering_others(self, tmp_path, monkeypatc assert written["other"] is True +class TestCopilotUserMcpConfig: + ENTRY = {"type": "local", "command": "ug", "args": ["mcp-proxy"], "tools": ["*"]} + + def test_honors_copilot_home(self, tmp_path, monkeypatch): + monkeypatch.setenv("COPILOT_HOME", str(tmp_path / "copilot-home")) + + copilot.write_user_mcp_servers({"svc": self.ENTRY}, set()) + + written = json.loads((tmp_path / "copilot-home" / "mcp-config.json").read_text()) + assert written["mcpServers"] == {"svc": self.ENTRY} + + def test_refuses_to_rewrite_a_config_it_cannot_parse(self): + path = copilot.copilot_mcp_config_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("{not json", encoding="utf-8") + + with pytest.raises(RuntimeError, match="isn't a valid JSON object"): + copilot.write_user_mcp_servers({"svc": self.ENTRY}, set()) + + assert path.read_text(encoding="utf-8") == "{not json" + + def test_moves_legacy_side_file_servers_into_the_user_config(self): + path = copilot.copilot_mcp_config_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps({"mcpServers": {"mine": {"x": 1}}, "other": True})) + copilot.LEGACY_MCP_CONFIG_PATH.write_text( + json.dumps({"mcpServers": {"svc": self.ENTRY, "gone": self.ENTRY}}) + ) + copilot.LEGACY_MCP_BACKUP_PATH.write_text("{}") + + removed = copilot.write_user_mcp_servers({}, {"gone"}) + + assert removed == {"gone"} + written = json.loads(path.read_text()) + assert written["other"] is True + assert written["mcpServers"] == {"mine": {"x": 1}, "svc": self.ENTRY} + assert not copilot.LEGACY_MCP_CONFIG_PATH.exists() + assert not copilot.LEGACY_MCP_BACKUP_PATH.exists() + + def test_migration_is_a_no_op_without_a_legacy_side_file(self): + copilot.migrate_legacy_mcp_config() + + assert not copilot.copilot_mcp_config_path().exists() + + def isolate_copilot_config_paths(monkeypatch, tmp_path): import ucode.config_io as config_io import ucode.state as state_mod @@ -916,7 +957,6 @@ def isolate_copilot_config_paths(monkeypatch, tmp_path): monkeypatch.setattr(state_mod, "STATE_PATH", app_dir / "state.json") monkeypatch.setattr(copilot, "COPILOT_ENV_PATH", env_path) monkeypatch.setattr(copilot, "COPILOT_BACKUP_PATH", app_dir / "copilot-ucode-env.backup") - monkeypatch.setattr(copilot, "COPILOT_MCP_CONFIG_PATH", tmp_path / "missing-mcp.json") monkeypatch.setattr(copilot, "COPILOT_CONFIG_DIR", env_path.parent) return env_path @@ -1139,7 +1179,14 @@ def test_explicit_model_is_rewritten_to_the_registry_ref(self, tmp_path, monkeyp options=copilot.LaunchOptions(launch_model=SONNET), ) - assert seen["argv"] == ["copilot", "-p", "hi", "--model", f"dbx-responses/{GPT}"] + assert seen["argv"] == [ + "copilot", + "--allow-all-mcp-server-instructions", + "-p", + "hi", + "--model", + f"dbx-responses/{GPT}", + ] assert seen["env"]["COPILOT_MODEL"] == f"dbx-responses/{GPT}" def test_starts_on_the_requested_launch_model(self, tmp_path, monkeypatch): @@ -1320,24 +1367,20 @@ def test_has_prompt_flag(self): cmd = copilot.validate_cmd("copilot") assert "--prompt" in cmd - def test_adds_ucode_mcp_config_when_present(self, tmp_path, monkeypatch): - mcp_path = tmp_path / "ucode-mcp-config.json" - mcp_path.write_text("{}", encoding="utf-8") - monkeypatch.setattr(copilot, "COPILOT_MCP_CONFIG_PATH", mcp_path) - + def test_lets_mcp_server_instructions_reach_the_model(self): cmd = copilot.validate_cmd("copilot") - assert cmd[:3] == ["copilot", "--additional-mcp-config", f"@{mcp_path}"] + assert cmd[:2] == ["copilot", "--allow-all-mcp-server-instructions"] + assert "--additional-mcp-config" not in cmd class TestWriteUserMcpServers: def test_batched_add_remove_preserves_other_keys(self, tmp_path, monkeypatch): - path = tmp_path / "ucode-mcp-config.json" + path = tmp_path / "mcp-config.json" path.write_text( json.dumps({"other": 1, "mcpServers": {"mine": {"type": "local"}, "gone": {}}}) ) monkeypatch.setattr(copilot, "COPILOT_MCP_CONFIG_PATH", path) - monkeypatch.setattr(copilot, "COPILOT_MCP_BACKUP_PATH", tmp_path / "backup.json") copilot.write_user_mcp_servers( {"svc": copilot.build_mcp_server_entry(["ug", "mcp-proxy", "u"])}, {"gone"} diff --git a/tests/test_integration_contract.py b/tests/test_integration_contract.py index 7f714c398..9399608f1 100644 --- a/tests/test_integration_contract.py +++ b/tests/test_integration_contract.py @@ -419,6 +419,7 @@ def test_cuj_discovery_fails_for_a_file_without_a_check_name(tmp_path): "antigravity", "managed", "managed-opencode", + "managed-copilot", "dedicated-cuj-plan", "dedicated-cuj", "cujs", diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 87dac057b..64b83fc12 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -3068,22 +3068,14 @@ def test_reports_schemaless_connection(self, capsys): class TestRevertMcpConfigs: - def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkeypatch): + def test_removes_cli_registered_servers_from_every_client(self, monkeypatch): removed: list[tuple[str, str]] = [] - restored: list[tuple[object, object, bool]] = [] monkeypatch.setattr( mcp, "remove_client_mcp_server", lambda client, name: removed.append((client, name)) or ["user"], ) - monkeypatch.setattr( - mcp, - "restore_file", - lambda config_path, backup_path, managed: ( - restored.append((config_path, backup_path, managed)) or True - ), - ) result = mcp.revert_mcp_configs( { @@ -3103,9 +3095,6 @@ def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkey ("opencode", "github-mcp"), ("copilot", "github-mcp"), ] - assert restored == [ - (mcp.copilot.COPILOT_MCP_CONFIG_PATH, mcp.copilot.COPILOT_MCP_BACKUP_PATH, True) - ] assert result == { "claude": True, "codex": True, @@ -3114,6 +3103,23 @@ def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkey "copilot": True, } + def test_removes_copilot_servers_from_its_user_config_and_the_legacy_side_file(self): + user_config = mcp.copilot.copilot_mcp_config_path() + user_config.parent.mkdir(parents=True, exist_ok=True) + user_config.write_text( + json.dumps({"mcpServers": {"github-mcp": {"type": "local"}, "mine": {"x": 1}}}) + ) + legacy = mcp.copilot.LEGACY_MCP_CONFIG_PATH + legacy.write_text(json.dumps({"mcpServers": {"stale-ug": {"type": "local"}}})) + + result = mcp.revert_mcp_configs( + {"mcp_servers": [{"name": "github-mcp", "clients": ["copilot"]}]} + ) + + assert result == {"copilot": True} + assert json.loads(user_config.read_text())["mcpServers"] == {"mine": {"x": 1}} + assert not legacy.exists() + def test_removes_skills_registry_across_its_clients(self, monkeypatch): removed: list[tuple[str, str]] = [] monkeypatch.setattr( @@ -3121,7 +3127,6 @@ def test_removes_skills_registry_across_its_clients(self, monkeypatch): "remove_client_mcp_server", lambda client, name: removed.append((client, name)) or ["user"], ) - monkeypatch.setattr(mcp, "restore_file", lambda *a, **kw: False) skills_entry = mcp._resolve_skills_mcp_servers( WS, ["claude", "codex"], _by_client(["claude", "codex"], ["a.b"]), [] @@ -4181,6 +4186,23 @@ def test_parses_codex_enabled_disabled_table(self): "github": mcp.LIVE_ENABLED, } + def test_parses_copilot_json_listing(self): + output = json.dumps( + { + "mcpServers": { + "system-ai-github": {"type": "local", "source": "user", "enabled": True}, + "paused": {"type": "local", "source": "user", "enabled": False}, + } + } + ) + assert mcp.parse_mcp_list_output("copilot", output) == { + "system-ai-github": mcp.LIVE_ENABLED, + "paused": mcp.LIVE_DISABLED, + } + + def test_unreadable_copilot_listing_returns_no_servers(self): + assert mcp.parse_mcp_list_output("copilot", "error: something broke") == {} + def test_empty_listing_returns_no_servers(self): assert mcp.parse_mcp_list_output("gemini", "No MCP servers configured.") == {} @@ -4224,14 +4246,6 @@ def test_cursor_unapproved_server_is_not_reported_missing(self): output = "Loading MCPs…\nug-audit: not loaded (needs approval)\n" assert mcp.parse_mcp_list_output("cursor", output) == {"ug-audit": mcp.LIVE_NEEDS_APPROVAL} - def test_copilot_additional_config_is_reported_as_enabled(self, tmp_path, monkeypatch): - # Bare `copilot mcp list` does not include servers from --additional-mcp-config. - config = tmp_path / "ucode-mcp-config.json" - config.write_text('{"mcpServers":{"ug-audit":{"type":"local"}}}') - monkeypatch.setattr(mcp.copilot, "COPILOT_MCP_CONFIG_PATH", config) - monkeypatch.setattr(mcp, "_run_mcp_list", lambda client: "No MCP servers configured.") - assert mcp.query_live_mcp_status("copilot") == {"ug-audit": mcp.LIVE_ENABLED} - def test_ansi_progress_escapes_are_stripped_before_parsing(self, monkeypatch): # cursor-agent redraws progress with ANSI escapes that must not leak into server names. class _Result: @@ -4494,6 +4508,15 @@ def test_codex_caveat_shown_only_when_codex_reported(self, monkeypatch, capsys): assert mcp.list_mcp_command() == 0 assert "Codex reports enabled/disabled" in _unwrap(capsys.readouterr().out) + def test_caveat_names_codex_and_copilot_when_both_reported(self, monkeypatch, capsys): + self._patch( + monkeypatch, installed=("claude", "codex", "copilot"), live={"codex": {}, "copilot": {}} + ) + assert mcp.list_mcp_command() == 0 + assert "Codex and GitHub Copilot CLI report enabled/disabled" in _unwrap( + capsys.readouterr().out + ) + def test_codex_caveat_omitted_when_scoped_out(self, monkeypatch, capsys): # Scoping Codex out drops the Codex clause from the note. self._patch(monkeypatch, live={"claude": {}, "codex": {}}) From 79c3e4703a9ccefa991e92942dfaa8630e433e47 Mon Sep 17 00:00:00 2001 From: Xi Lyu Date: Mon, 26 Oct 2026 14:12:38 +0000 Subject: [PATCH 02/11] copilot: edit commented MCP configs through the copilot CLI; keep the side file until the write lands - Copilot accepts JSON with comments in mcp-config.json. When ug can't parse the file, it now applies just the changed entries with `copilot mcp remove` / `copilot mcp add`, as it does for Claude Code and Codex, instead of refusing. A file the CLI can't read either is still left alone. - The legacy side file is deleted only after the user config is written, so a failed write no longer loses ug's servers. An empty side file is still removed. - `ug mcp list` reads only the leading JSON object of `copilot mcp list --json`, so stderr after it no longer hides every server. - The managed MCP journey looks up `system-ai-github` by name. Co-authored-by: Isaac --- src/ucode/agents/copilot.py | 88 +++++++++++++------ src/ucode/config_io.py | 2 +- src/ucode/mcp.py | 12 ++- .../test_ug_configure_managed_mcp.py | 5 +- tests/test_agent_copilot.py | 67 +++++++++++++- tests/test_mcp.py | 4 + 6 files changed, 142 insertions(+), 36 deletions(-) diff --git a/src/ucode/agents/copilot.py b/src/ucode/agents/copilot.py index 34f8c6139..c9f85e7f9 100644 --- a/src/ucode/agents/copilot.py +++ b/src/ucode/agents/copilot.py @@ -29,6 +29,7 @@ import os import re import secrets +import subprocess import threading import time from collections.abc import Callable, Iterable, Iterator @@ -803,30 +804,25 @@ def build_mcp_server_entry(argv: list[str]) -> dict: def copilot_mcp_config_path() -> Path: - """The user config Copilot reads ``mcpServers`` from (and ``copilot mcp add`` writes): - ``$COPILOT_HOME/mcp-config.json`` when that env var is set, else ``~/.copilot/mcp-config.json``.""" + """The user config Copilot reads ``mcpServers`` from (and ``copilot mcp add`` writes).""" copilot_home = os.environ.get("COPILOT_HOME") return Path(copilot_home) / "mcp-config.json" if copilot_home else COPILOT_MCP_CONFIG_PATH -def _read_mcp_config_for_rewrite(path: Path) -> dict: - """``path`` parsed for a rewrite (``{}`` when absent). Raises rather than return ``{}`` for a - file that isn't a JSON object, since rewriting it would drop the developer's own servers.""" +def _read_mcp_config_for_rewrite(path: Path) -> dict | None: + """``path`` parsed for a rewrite: ``{}`` when absent, ``None`` when it isn't a JSON object, so + a caller never overwrites it. Copilot also accepts JSON with comments, which lands here.""" if not path.exists(): return {} try: data = json.loads(path.read_text(encoding="utf-8")) except (OSError, UnicodeError, json.JSONDecodeError): - data = None - if not isinstance(data, dict): - raise RuntimeError( - f"Can't update {path}: it isn't a valid JSON object. Fix it, then retry." - ) - return data + return None + return data if isinstance(data, dict) else None def remove_legacy_mcp_config() -> bool: - """Delete the legacy side file and its backup (on revert: everything in them is ug's).""" + """Delete the legacy side file and its backup (everything in them is ug's).""" existed = LEGACY_MCP_CONFIG_PATH.exists() or LEGACY_MCP_BACKUP_PATH.exists() if is_dry_run(): return existed @@ -835,32 +831,74 @@ def remove_legacy_mcp_config() -> bool: return existed -def _take_legacy_mcp_servers() -> dict[str, dict]: - """The servers in the legacy side file, deleting it and its backup. Every entry there was - written by ug, so it moves into the user config as-is.""" +def _read_legacy_mcp_servers() -> dict[str, dict]: + """The servers in the legacy side file. Every entry there was written by ug, so it moves into + the user config as-is.""" servers = read_json_safe(LEGACY_MCP_CONFIG_PATH).get("mcpServers") - remove_legacy_mcp_config() if not isinstance(servers, dict): return {} return {name: entry for name, entry in servers.items() if isinstance(entry, dict)} +def _remove_mcp_server_via_cli(name: str) -> bool: + try: + result = subprocess_cross_os.run( + [SPEC["binary"], "mcp", "remove", name], + check=False, + capture_output=True, + text=True, + timeout=30, + ) + except subprocess.TimeoutExpired as exc: + raise RuntimeError(f"Timed out removing MCP server '{name}' via copilot CLI.") from exc + if "not found" in f"{result.stderr or ''}\n{result.stdout or ''}".lower(): + return False + if result.returncode != 0: + raise RuntimeError(f"Failed to remove MCP server '{name}' via copilot CLI.") + return True + + +def _add_mcp_server_via_cli(name: str, entry: dict) -> None: + # The CLI refuses an existing name, so a replace removes it first. + _remove_mcp_server_via_cli(name) + argv = [entry["command"], *entry.get("args", [])] + try: + subprocess_cross_os.run( + [SPEC["binary"], "mcp", "add", name, "--", *argv], + check=True, + capture_output=True, + text=True, + timeout=30, + ) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as exc: + raise RuntimeError(f"Failed to add MCP server '{name}' via copilot CLI.") from exc + + def _apply_mcp_diff(add: dict[str, dict], remove: set[str]) -> set[str]: path = copilot_mcp_config_path() config = _read_mcp_config_for_rewrite(path) + legacy = _read_legacy_mcp_servers() + upserts = {name: entry for name, entry in legacy.items() if name not in remove} | add + if config is None: + # Defer to the copilot CLI, which reads the file the way Copilot does, for exactly the + # changed entries. + removed = {name for name in remove if _remove_mcp_server_via_cli(name) or name in legacy} + for name, entry in upserts.items(): + _add_mcp_server_via_cli(name, entry) + remove_legacy_mcp_config() + return removed + servers = config.get("mcpServers") if not isinstance(servers, dict): servers = {} - legacy = _take_legacy_mcp_servers() removed = {name for name in remove if name in servers or name in legacy} - if not add and not removed and not legacy: - return removed - servers.update({name: entry for name, entry in legacy.items() if name not in remove}) - for name in remove: - servers.pop(name, None) - servers.update(add) - config["mcpServers"] = servers - write_json_file(path, config) + if upserts or removed: + for name in remove: + servers.pop(name, None) + servers.update(upserts) + config["mcpServers"] = servers + write_json_file(path, config) + remove_legacy_mcp_config() return removed diff --git a/src/ucode/config_io.py b/src/ucode/config_io.py index c4ca3578c..85ce794ac 100644 --- a/src/ucode/config_io.py +++ b/src/ucode/config_io.py @@ -185,7 +185,7 @@ def apply_json_mcp_diff( ) -> set[str]: """Apply an ``add``/``remove`` MCP-server diff to the ``key`` map in a JSON config, in one read-modify-write. Shared by the agents whose MCP servers live in a JSON object keyed by name - (copilot/cursor/gemini/opencode); the developer's own entries and every other key are kept. + (cursor/gemini/kilo/opencode); the developer's own entries and every other key are kept. ``backup_path`` snapshots the file first when the agent keeps a revert backup. Returns the subset of ``remove`` names that were actually present (so callers can report only real removals).""" if backup_path is not None: diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index f9d342d1a..85a26e55a 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -2507,12 +2507,16 @@ def _parse_tree_mcp_list(output: str) -> dict[str, str]: def _parse_copilot_mcp_list(output: str) -> dict[str, str]: """Parse `copilot mcp list --json` into ``{server_name: state}``. - Like Codex, Copilot reports config state (``enabled``), not a health probe. Output that isn't - the expected JSON object contributes nothing.""" + Like Codex, Copilot reports config state (``enabled``), not a health probe. Only the leading + JSON object is read, since stderr follows it; output without one contributes nothing.""" + start = output.find("{") + if start < 0: + return {} try: - servers = json.loads(output[output.find("{") :]).get("mcpServers") - except (json.JSONDecodeError, AttributeError): + data, _ = json.JSONDecoder().raw_decode(output, start) + except json.JSONDecodeError: return {} + servers = data.get("mcpServers") if isinstance(data, dict) else None if not isinstance(servers, dict): return {} return { diff --git a/tests/integration/test_ug_configure_managed_mcp.py b/tests/integration/test_ug_configure_managed_mcp.py index 2c94f5019..072f32586 100644 --- a/tests/integration/test_ug_configure_managed_mcp.py +++ b/tests/integration/test_ug_configure_managed_mcp.py @@ -133,9 +133,8 @@ def test_managed_fixture_copilot_mcp_registered_in_user_config(live_session, wor listing = session.run("mcp", "list", "--json", binary=session.which("copilot"), timeout=60) servers = json.loads(listing.stdout)["mcpServers"] - entries = {name: entry for name, entry in servers.items() if "github" in name.lower()} - assert entries, servers - entry = next(iter(entries.values())) + assert "system-ai-github" in servers, servers + entry = servers["system-ai-github"] assert entry["source"] == "user", entry assert entry["enabled"] is True, entry assert entry["command"].endswith("ug"), entry diff --git a/tests/test_agent_copilot.py b/tests/test_agent_copilot.py index 82f682cd5..3f102ddcd 100644 --- a/tests/test_agent_copilot.py +++ b/tests/test_agent_copilot.py @@ -5,6 +5,7 @@ import contextlib import json import os +import subprocess from pathlib import Path import httpx @@ -912,15 +913,75 @@ def test_honors_copilot_home(self, tmp_path, monkeypatch): written = json.loads((tmp_path / "copilot-home" / "mcp-config.json").read_text()) assert written["mcpServers"] == {"svc": self.ENTRY} - def test_refuses_to_rewrite_a_config_it_cannot_parse(self): + @staticmethod + def _stub_cli(monkeypatch, *, fail_add=False): + calls: list[list[str]] = [] + + def run(argv, **kwargs): + calls.append(argv) + if argv[2] == "add" and fail_add: + raise subprocess.CalledProcessError(1, argv) + stderr = f'Server "{argv[3]}" not found.' if argv[2] == "remove" else "" + return subprocess.CompletedProcess(argv, 1 if stderr else 0, "", stderr) + + monkeypatch.setattr(copilot.subprocess_cross_os, "run", run) + return calls + + def test_defers_to_the_copilot_cli_for_a_config_it_cannot_parse(self, monkeypatch): + calls = self._stub_cli(monkeypatch) + path = copilot.copilot_mcp_config_path() + path.parent.mkdir(parents=True, exist_ok=True) + jsonc = '{\n // mine\n "mcpServers": {"mine": {"x": 1},},\n}\n' + path.write_text(jsonc, encoding="utf-8") + copilot.LEGACY_MCP_CONFIG_PATH.write_text(json.dumps({"mcpServers": {"old": self.ENTRY}})) + + removed = copilot.write_user_mcp_servers({"svc": self.ENTRY}, {"gone"}) + + assert removed == set() + assert calls == [ + ["copilot", "mcp", "remove", "gone"], + ["copilot", "mcp", "remove", "old"], + ["copilot", "mcp", "add", "old", "--", "ug", "mcp-proxy"], + ["copilot", "mcp", "remove", "svc"], + ["copilot", "mcp", "add", "svc", "--", "ug", "mcp-proxy"], + ] + assert path.read_text(encoding="utf-8") == jsonc + assert not copilot.LEGACY_MCP_CONFIG_PATH.exists() + + def test_failed_cli_add_keeps_the_legacy_side_file(self, monkeypatch): + self._stub_cli(monkeypatch, fail_add=True) path = copilot.copilot_mcp_config_path() path.parent.mkdir(parents=True, exist_ok=True) path.write_text("{not json", encoding="utf-8") + copilot.LEGACY_MCP_CONFIG_PATH.write_text(json.dumps({"mcpServers": {"old": self.ENTRY}})) - with pytest.raises(RuntimeError, match="isn't a valid JSON object"): - copilot.write_user_mcp_servers({"svc": self.ENTRY}, set()) + with pytest.raises(RuntimeError, match="Failed to add MCP server 'old'"): + copilot.migrate_legacy_mcp_config() assert path.read_text(encoding="utf-8") == "{not json" + assert copilot.LEGACY_MCP_CONFIG_PATH.exists() + + def test_failed_write_keeps_the_legacy_side_file(self, monkeypatch): + def fail(path, payload): + raise RuntimeError(f"Failed to write config file: {path}") + + monkeypatch.setattr(copilot, "write_json_file", fail) + copilot.LEGACY_MCP_CONFIG_PATH.parent.mkdir(parents=True, exist_ok=True) + copilot.LEGACY_MCP_CONFIG_PATH.write_text(json.dumps({"mcpServers": {"old": self.ENTRY}})) + + with pytest.raises(RuntimeError, match="Failed to write config file"): + copilot.migrate_legacy_mcp_config() + + assert copilot.LEGACY_MCP_CONFIG_PATH.exists() + + def test_empty_legacy_side_file_is_removed_without_a_write(self): + copilot.LEGACY_MCP_CONFIG_PATH.parent.mkdir(parents=True, exist_ok=True) + copilot.LEGACY_MCP_CONFIG_PATH.write_text("{}") + + copilot.migrate_legacy_mcp_config() + + assert not copilot.LEGACY_MCP_CONFIG_PATH.exists() + assert not copilot.copilot_mcp_config_path().exists() def test_moves_legacy_side_file_servers_into_the_user_config(self): path = copilot.copilot_mcp_config_path() diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 64b83fc12..348731e91 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -4200,6 +4200,10 @@ def test_parses_copilot_json_listing(self): "paused": mcp.LIVE_DISABLED, } + def test_copilot_listing_followed_by_stderr_still_parses(self): + output = json.dumps({"mcpServers": {"svc": {"enabled": True}}}) + "\nwarning: slow start\n" + assert mcp.parse_mcp_list_output("copilot", output) == {"svc": mcp.LIVE_ENABLED} + def test_unreadable_copilot_listing_returns_no_servers(self): assert mcp.parse_mcp_list_output("copilot", "error: something broke") == {} From a95f54f2b1c1984905d9fd99ccd704546d93ebaa Mon Sep 17 00:00:00 2001 From: Xi Lyu Date: Mon, 26 Oct 2026 14:35:56 +0000 Subject: [PATCH 03/11] mcp: keep reverting the other agents when one agent's MCP removal fails - `ug revert` now warns and moves on when removing a server from one agent fails (for example a Copilot config neither ug nor the copilot CLI can read), instead of stopping before the other agents and the rest of the revert. - Deleting Copilot's legacy side file raises a RuntimeError rather than a raw OSError, so `ug copilot` warns and still launches when it can't. Co-authored-by: Isaac --- src/ucode/agents/copilot.py | 7 +++++-- src/ucode/mcp.py | 15 ++++++++++++--- tests/test_agent_copilot.py | 12 ++++++++++++ tests/test_mcp.py | 21 +++++++++++++++++++++ 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/ucode/agents/copilot.py b/src/ucode/agents/copilot.py index c9f85e7f9..be2e57353 100644 --- a/src/ucode/agents/copilot.py +++ b/src/ucode/agents/copilot.py @@ -826,8 +826,11 @@ def remove_legacy_mcp_config() -> bool: existed = LEGACY_MCP_CONFIG_PATH.exists() or LEGACY_MCP_BACKUP_PATH.exists() if is_dry_run(): return existed - LEGACY_MCP_CONFIG_PATH.unlink(missing_ok=True) - LEGACY_MCP_BACKUP_PATH.unlink(missing_ok=True) + for path in (LEGACY_MCP_CONFIG_PATH, LEGACY_MCP_BACKUP_PATH): + try: + path.unlink(missing_ok=True) + except OSError as exc: + raise RuntimeError(f"Failed to remove {path}") from exc return existed diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 85a26e55a..a244a1357 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -457,8 +457,12 @@ def revert_mcp_configs(state: dict) -> dict[str, bool]: all_servers = list(state.get("mcp_servers") or []) + list( state.get("managed_mcp_servers") or [] ) - if copilot.remove_legacy_mcp_config(): - results["copilot"] = True + # One agent's unreadable config must not stop the revert of the others. + try: + if copilot.remove_legacy_mcp_config(): + results["copilot"] = True + except RuntimeError as exc: + print_warning(f"Could not remove ug's old Copilot MCP file: {exc}") for server in all_servers: name = server.get("name") if not isinstance(name, str) or not name: @@ -466,7 +470,12 @@ def revert_mcp_configs(state: dict) -> dict[str, bool]: for client in server.get("clients") or []: if client not in MCP_CLIENTS: continue - removed_scopes = remove_client_mcp_server(client, name) + try: + removed_scopes = remove_client_mcp_server(client, name) + except RuntimeError as exc: + display = MCP_CLIENTS[client]["display"] + print_warning(f"Could not remove MCP server `{name}` from {display}: {exc}") + continue results[client] = bool(removed_scopes) or results.get(client, False) return results diff --git a/tests/test_agent_copilot.py b/tests/test_agent_copilot.py index 3f102ddcd..f416b1ad2 100644 --- a/tests/test_agent_copilot.py +++ b/tests/test_agent_copilot.py @@ -974,6 +974,18 @@ def fail(path, payload): assert copilot.LEGACY_MCP_CONFIG_PATH.exists() + def test_a_legacy_side_file_that_cannot_be_deleted_raises_a_runtime_error(self, monkeypatch): + copilot.LEGACY_MCP_CONFIG_PATH.parent.mkdir(parents=True, exist_ok=True) + copilot.LEGACY_MCP_CONFIG_PATH.write_text("{}") + + def unlink(self, missing_ok=False): + raise PermissionError(13, "Permission denied") + + monkeypatch.setattr(Path, "unlink", unlink) + + with pytest.raises(RuntimeError, match="Failed to remove"): + copilot.remove_legacy_mcp_config() + def test_empty_legacy_side_file_is_removed_without_a_write(self): copilot.LEGACY_MCP_CONFIG_PATH.parent.mkdir(parents=True, exist_ok=True) copilot.LEGACY_MCP_CONFIG_PATH.write_text("{}") diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 348731e91..6019ce09a 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -3103,6 +3103,27 @@ def test_removes_cli_registered_servers_from_every_client(self, monkeypatch): "copilot": True, } + def test_one_agents_failed_removal_does_not_stop_the_others(self, monkeypatch, capsys): + removed: list[tuple[str, str]] = [] + + def remove(client, name): + if client == "copilot": + raise RuntimeError("Failed to remove MCP server via copilot CLI.") + removed.append((client, name)) + return ["user"] + + monkeypatch.setattr(mcp, "remove_client_mcp_server", remove) + + result = mcp.revert_mcp_configs( + {"mcp_servers": [{"name": "svc", "clients": ["copilot", "claude", "codex"]}]} + ) + + assert removed == [("claude", "svc"), ("codex", "svc")] + assert result == {"claude": True, "codex": True} + assert "Could not remove MCP server `svc` from GitHub Copilot CLI" in _unwrap( + capsys.readouterr().out + ) + def test_removes_copilot_servers_from_its_user_config_and_the_legacy_side_file(self): user_config = mcp.copilot.copilot_mcp_config_path() user_config.parent.mkdir(parents=True, exist_ok=True) From 15a89d318846a4831b6198976c5a7078c4be9e3d Mon Sep 17 00:00:00 2001 From: Xi Lyu Date: Fri, 30 Oct 2026 12:18:05 +0000 Subject: [PATCH 04/11] tests: move the managed Copilot lane to the managed-config follow-up The gateway does not accept CODING_AGENT_COPILOT managed configs, so the lane only exercised a config the stub injects. The personal journey test_ug_copilot_mcp.py covers the user-config writer end to end. Co-authored-by: Isaac --- .github/workflows/integration.yml | 57 ------------------- tests/AGENTS.md | 8 +-- tests/README.md | 1 - .../fixtures/managed_config/copilot_mcp.json | 24 -------- .../copilot_skills_by_name.json | 24 -------- tests/integration/README.md | 17 ++---- .../test_ug_configure_managed_mcp.py | 27 +-------- .../test_ug_configure_managed_skills.py | 27 +-------- tests/test_integration_contract.py | 1 - 9 files changed, 13 insertions(+), 173 deletions(-) delete mode 100644 tests/fixtures/managed_config/copilot_mcp.json delete mode 100644 tests/fixtures/managed_config/copilot_skills_by_name.json diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 77d02654b..b5911fe56 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -478,63 +478,6 @@ jobs: ${{ runner.temp }}/ug-integration/artifacts/ ${{ runner.temp }}/ug-integration/wheels/ - # Non-blocking like the Copilot lane: continue-on-error and absent from the cujs `needs`. - # Runs the Copilot managed_fixture cases on the managed e2e workspace with injected configs that - # enable Copilot (a service principal authenticates it, like the `managed` lane). - managed-copilot: - name: Managed config · Copilot (non-blocking) - if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }} - continue-on-error: true - runs-on: ubuntu-22.04 - timeout-minutes: 30 - env: - UCODE_TEST_WORKSPACE: ${{ secrets.E2E_ADMIN_WORKSPACE }} - DATABRICKS_CLIENT_ID: ${{ secrets.E2E_ADMIN_SP_CLIENT_ID }} - DATABRICKS_CLIENT_SECRET: ${{ secrets.E2E_ADMIN_SP_CLIENT_SECRET }} - DEPENDENCY: ${{ inputs.dependency }} - TEST_MARKER: managed_fixture and copilot - ARTIFACT_NAME: integration-managed-copilot - steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - with: - ref: ${{ inputs.ref || github.sha }} - persist-credentials: false - fetch-depth: 0 - - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 - with: - node-version: 22.19.0 - - uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0 - with: - version: 0.9.8 - enable-cache: ${{ inputs.ref == '' }} - - uses: databricks/setup-cli@bdb89f81c11a5bd647fd55b585b7c396ec68a25a # v1.0.0 - with: - version: 1.17.0 - - name: Run the Copilot managed cases against the managed workspace - shell: bash - run: | - args=(--copilot-version "$COPILOT_VERSION") - if [[ -n "$DEPENDENCY" ]]; then - args+=(--dependency "$DEPENDENCY") - fi - uv run --no-project --python 3.12 python scripts/run_integration.py \ - --python 3.12 --ug-version "$UG_VERSION" --entry-point "$ENTRY_POINT" \ - --default-index "$PACKAGE_INDEX" --output "$RUNNER_TEMP/ug-integration" \ - "${args[@]}" -- -m "$TEST_MARKER" - - name: Upload managed Copilot evidence - if: ${{ !cancelled() }} - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 - with: - name: ${{ env.ARTIFACT_NAME }} - include-hidden-files: true - path: | - ${{ runner.temp }}/ug-integration/*.json - ${{ runner.temp }}/ug-integration/*.txt - ${{ runner.temp }}/ug-integration/*.xml - ${{ runner.temp }}/ug-integration/*.log - ${{ runner.temp }}/ug-integration/artifacts/ - ${{ runner.temp }}/ug-integration/wheels/ - # managed and dedicated-cuj stay Linux-only: ug has no Windows managed-settings # writer, and the CUJ suite drives agents through a POSIX PTY. managed: diff --git a/tests/AGENTS.md b/tests/AGENTS.md index e2fbe07da..4c16701f5 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -93,10 +93,10 @@ exercise. and run documentation at the suite root. - Current scope is basic Claude/Codex configuration, routing, script usage, command forwarding, and configure/revert CUJs, plus Copilot CLI headless - model tasks, one TUI model-switch journey, and Copilot MCP/skills journeys matching the - Claude/Codex ones (non-blocking lanes), plus Antigravity CLI configure, TUI, headless, - External Models, MCP-tool, skill-directory, and revert journeys (blocking Linux lane). - Do not add other MCP/skills functionality, + model tasks, one TUI model-switch journey, and one MCP add, tool-call and revert journey + (non-blocking lane), plus Antigravity CLI configure, TUI, headless, External Models, + MCP-tool, skill-directory, and revert journeys (blocking Linux lane). Do not add other + MCP/skills functionality, tracing, or the broad configure-option matrix without a new scope request. Existing `mcp --help` checks cover dispatch only. diff --git a/tests/README.md b/tests/README.md index d7438fc49..8c8714789 100644 --- a/tests/README.md +++ b/tests/README.md @@ -679,7 +679,6 @@ integration utilities; only CUJ-specific evidence correlation stays in a test fi | `test_ug_copilot_refreshes_a_revoked_token_mid_session`, `test_ug_copilot_reports_expired_sign_in` | Sign in through `DATABRICKS_BEARER_COMMAND` minting short-lived PATs; mid-task the agent revokes them (or also signs out) | Revoked: a token is minted after the revocation and the task completes without restarting Copilot. Signed out: output shows "Databricks sign-in expired" with `databricks auth login`; the failure is in `~/.ucode/logs/copilot-proxy.log`, not the terminal (non-blocking CI lane) | | `test_ug_configure_copilot_repeat_and_revert` | Configure Copilot twice over a user-owned `~/.copilot/mcp-config.json`, run a GPT task, revert twice | Task completes; no file holds the bearer; revert removes `~/.copilot/ucode.env`, leaves the user file byte-identical and reports Not Configured. (non-blocking CI lane) | | `test_ug_mcp_add_copilot_registers_server_in_copilot_config` | Over a developer-owned, disabled server in `~/.copilot/mcp-config.json`, configure Copilot, `ug mcp add --agents copilot --names system.ai.web_search`, run a headless task that calls the tool, then revert | `copilot mcp list --json` lists ug's server from the user config, enabled, next to the developer's still-disabled one; `ug mcp list --agents copilot` reports it enabled; Copilot's session events record a successful call loaded from the user config; revert leaves only the developer's server and no legacy `ucode-mcp-config.json` (non-blocking CI lane) | -| `test_managed_fixture_copilot_mcp_registered_in_user_config`, `test_managed_fixture_copilot_skills_lists_downloaded_skill` | Non-interactive configure with an injected config enabling Copilot with a managed MCP server or a managed skill | `copilot mcp list --json` lists the managed server from the user config as the ug mcp-proxy command; `copilot skill list --json` lists the downloaded skill as an enabled personal skill (non-blocking managed Copilot lane) | | `test_ug_copilot_upgrade_scrubs_token_saved_by_older_ug` | Configure Copilot with ug v0.2.0 (pinned commit; saves the bearer in `~/.copilot/ucode.env`), then configure with this ug, run a GPT task, revert | v0.2.0 left the bearer; after this ug's configure no file holds it; task completes; revert leaves no token and no `ucode.env` (non-blocking CI lane) | | `test_ug_opencode_headless_prompt_argument` | Run OpenCode from a script (`run --format json --auto`) with an argument prompt | Completed Read tool call; final text answer contains the file value; exit zero (non-blocking CI lane) | | `test_ug_agents_self_managed_opencode_journey` | Under the injected `managed_workspace_default` config (enables Claude/Codex, not OpenCode): bare configure, `ug agents list`, refused OpenCode launch, `ug agents add opencode`, real headless task, `ug agents remove opencode` (marker `managed_fixture and opencode`, non-blocking CI lane) | OpenCode absent from the list and launch fails with "doesn't enable OpenCode" / `ug agents add opencode`; after add it is listed self-managed and the headless Read task returns the fixture value; after remove it is hidden and refused again | diff --git a/tests/fixtures/managed_config/copilot_mcp.json b/tests/fixtures/managed_config/copilot_mcp.json deleted file mode 100644 index 8cec6a864..000000000 --- a/tests/fixtures/managed_config/copilot_mcp.json +++ /dev/null @@ -1,24 +0,0 @@ -{ - "spec_version": 1, - "default_agent": "CODING_AGENT_COPILOT", - "enabled_agents": [ - { - "agent": "CODING_AGENT_COPILOT", - "config": { - "models": { - "model_services": [ - "system.ai.claude-sonnet-4-6" - ] - }, - "default_models": { - "default_model": "system.ai.claude-sonnet-4-6" - } - } - } - ], - "mcp_servers": { - "names": [ - "system.ai.github" - ] - } -} diff --git a/tests/fixtures/managed_config/copilot_skills_by_name.json b/tests/fixtures/managed_config/copilot_skills_by_name.json deleted file mode 100644 index 0d6c2d02b..000000000 --- a/tests/fixtures/managed_config/copilot_skills_by_name.json +++ /dev/null @@ -1,24 +0,0 @@ -{ - "spec_version": 1, - "default_agent": "CODING_AGENT_COPILOT", - "enabled_agents": [ - { - "agent": "CODING_AGENT_COPILOT", - "config": { - "models": { - "model_services": [ - "system.ai.claude-sonnet-4-6" - ] - }, - "default_models": { - "default_model": "system.ai.claude-sonnet-4-6" - } - } - } - ], - "skills": { - "names": [ - "main.default.forkable-meals" - ] - } -} diff --git a/tests/integration/README.md b/tests/integration/README.md index f57d1fea5..8e1ea2203 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -340,10 +340,7 @@ Anthropic service's model, on the OpenAI, Azure OpenAI and Microsoft Foundry ser on a named model of the Bedrock allow-all service; launching on the relayed service fails before Copilot starts. `test_ug_copilot_mcp.py` adds `system.ai.web_search` with `ug mcp add --agents copilot` over a developer's own disabled server, checks Copilot's `copilot mcp list --json` and `ug mcp list`, runs -a headless task whose tool call Copilot's session events record as successful, and reverts. The -Copilot cases in `test_ug_configure_managed_mcp.py` and `test_ug_configure_managed_skills.py` check -that a managed server and a managed skill reach Copilot (`copilot mcp list --json`, `copilot skill -list --json`). +a headless task whose tool call Copilot's session events record as successful, and reverts. Antigravity CLI has five live headless journeys in `test_ug_antigravity_headless.py` and three in `test_ug_configure_antigravity.py`. Headless runs use `agy -p --output-format stream-json`; the @@ -690,8 +687,8 @@ test_ug_claude_model_discovery.py # unmanaged scenarios 9, 11, 13 test_ug_codex_model_discovery.py # unmanaged scenarios 10, 12, 14 test_ug_configure_managed.py # stubbed static model list/catalog pointer, no agent selector, saved-config replay test_ug_configure_managed_models.py # stubbed model sources, smart-routing banner, Codex fallback metadata -test_ug_configure_managed_mcp.py # stubbed managed MCP list (Claude, Codex, Copilot) -test_ug_configure_managed_skills.py # stubbed managed skills: download, coexist, reconcile away (Claude, Codex, Copilot) +test_ug_configure_managed_mcp.py # stubbed managed MCP list +test_ug_configure_managed_skills.py # stubbed managed skills: download, coexist, reconcile away test_ug_configure_managed_lifecycle.py # none -> A -> B -> MPS -> none: reconcile, clear on MPS/no-config test_installation.py # fresh installed package utils/ # process/terminal/evidence helpers and Docker files @@ -882,9 +879,9 @@ cases instead complete real file tasks through `--workspace` and `--model-locati journeys check setup and launch with existing services, without inference. There are **88 live cases** (including 12 marked TUI journeys) and **7 installation -checks** with Claude and Codex; selecting OpenCode adds one live headless case and two `managed_fixture` cases; selecting Copilot adds eighteen live cases and two `managed_fixture` cases; selecting Antigravity adds eight live cases (one TUI). One +checks** with Claude and Codex; selecting OpenCode adds one live headless case and two `managed_fixture` cases; selecting Copilot adds eighteen live cases; selecting Antigravity adds eight live cases (one TUI). One **`workspace_switch` case** uses two real workspaces and checks skills MCP cleanup and a completed -Claude task. A further **46 `managed_fixture` cases** (twelve of them also `live`) run on the +Claude task. A further **44 `managed_fixture` cases** (twelve of them also `live`) run on the managed workspace with a checked-in JSON CodingAgentConfig from `tests/fixtures/managed_config/` injected through `UCODE_MANAGED_CONFIG_STUB`; there are no cases that read a published config. Twelve explicit configured/fresh Claude and Codex discovery and source-override journeys use the @@ -1109,9 +1106,7 @@ in `test_ug_agents_self_managed.py`), installs Claude, Codex and OpenCode and ru managed e2e workspace (`E2E_ADMIN_WORKSPACE`) with the injected `managed_workspace_default` config, which enables Claude/Codex but not OpenCode. It is likewise outside `cujs`. A non-blocking **Copilot** job (`live and copilot`, seventeen cases) runs on Linux and Windows the -same way and is likewise outside `cujs` until it is stable. A non-blocking **Managed config · Copilot** -job (`managed_fixture and copilot`, two cases) installs only Copilot and runs on the managed e2e -workspace with injected configs that enable Copilot; it is likewise outside `cujs`. +same way and is likewise outside `cujs` until it is stable. A blocking **Antigravity** job (`live and antigravity`, eight cases) runs on Linux only with the pinned `ANTIGRAVITY_BUILD`; it is part of the `cujs` gate. diff --git a/tests/integration/test_ug_configure_managed_mcp.py b/tests/integration/test_ug_configure_managed_mcp.py index 072f32586..bd158cae2 100644 --- a/tests/integration/test_ug_configure_managed_mcp.py +++ b/tests/integration/test_ug_configure_managed_mcp.py @@ -10,11 +10,9 @@ the developer's own ~/.codex/config.toml servers. A non-interactive configure has no sudo reconcile, so it falls back to the user-scope registration, which the real /mcp view still lists. Claude's `managedMcpServers` path is covered by unit tests; its integration coverage needs the managed -workspace to publish the `claude-code` OAuth client, which E2E_ADMIN_WORKSPACE does not. Copilot -has no OS-managed MCP file, so its managed servers land in its user config, ~/.copilot/mcp-config.json. +workspace to publish the `claude-code` OAuth client, which E2E_ADMIN_WORKSPACE does not. """ -import json import tomllib import pytest @@ -116,26 +114,3 @@ def test_managed_fixture_codex_http_headers_in_managed_file(live_session, worksp f"Expected {managed_header_key!r} = {managed_header_value!r}, " f"got {headers[managed_header_key]!r}" ) - - -@pytest.mark.managed_fixture -@pytest.mark.copilot -def test_managed_fixture_copilot_mcp_registered_in_user_config(live_session, workspace): - """Scenario: a non-interactive configure with a managed MCP server enabled for Copilot. - - Expected: Copilot's own `copilot mcp list --json` lists the server from its user config, - enabled, as the ug mcp-proxy stdio command for the server's mcp-services URL. - """ - session = live_session - use_managed_config_fixture(session, "copilot_mcp") - result = session.run("configure", "--workspace", workspace, "--skip-upgrade", timeout=240) - assert "Select coding agents to configure:" not in result.stdout, result.stdout - - listing = session.run("mcp", "list", "--json", binary=session.which("copilot"), timeout=60) - servers = json.loads(listing.stdout)["mcpServers"] - assert "system-ai-github" in servers, servers - entry = servers["system-ai-github"] - assert entry["source"] == "user", entry - assert entry["enabled"] is True, entry - assert entry["command"].endswith("ug"), entry - assert "mcp-services" in entry["args"][entry["args"].index("--url") + 1], entry diff --git a/tests/integration/test_ug_configure_managed_skills.py b/tests/integration/test_ug_configure_managed_skills.py index f650c1e4d..9a96eda57 100644 --- a/tests/integration/test_ug_configure_managed_skills.py +++ b/tests/integration/test_ug_configure_managed_skills.py @@ -3,14 +3,12 @@ The admin CodingAgentConfig is a checked-in JSON fixture injected via UCODE_MANAGED_CONFIG_STUB so the real /skills TUI can be driven against a managed skill; only the config INPUT is stubbed (auth, the skill download, the config writers, and the agent binary stay real). These assert what the agent presents, not the bundles on disk (that is unit tests' job). See -tests/AGENTS.md rule 4. Claude reads ~/.claude/skills and Codex and Copilot read the shared -~/.agents/skills, both of which `ug configure` writes, so a managed skill reaches each agent. The reconcile +tests/AGENTS.md rule 4. Claude reads ~/.claude/skills and Codex reads the shared ~/.agents/skills, +both of which `ug configure` writes, so a managed skill reaches either agent. The reconcile lifecycle test additionally checks the on-disk bundles, since removal-on-reconcile is a disk change that has no steady-state `/skills` signal. """ -import json - import pytest from utils.managed import use_managed_config_fixture from utils.terminal import AgentTerminal @@ -81,27 +79,6 @@ def test_managed_fixture_codex_skills_lists_downloaded_skill(live_session, works ) -@pytest.mark.managed_fixture -@pytest.mark.copilot -def test_managed_fixture_copilot_skills_lists_downloaded_skill(live_session, workspace): - """Scenario: configure Copilot under an injected config that names a managed skill, then list - Copilot's skills. - - Expected: Copilot's own `copilot skill list --json` lists the downloaded managed skill as an - enabled personal skill, read from the shared ~/.agents/skills dir `ug configure` writes. - """ - session = live_session - use_managed_config_fixture(session, "copilot_skills_by_name") - result = session.run("configure", "--workspace", workspace, "--skip-upgrade", timeout=240) - assert "Select coding agents to configure:" not in result.stdout, result.stdout - - listing = session.run("skill", "list", "--json", binary=session.which("copilot"), timeout=60) - skills = [skill for skill in json.loads(listing.stdout) if skill.get("name") == SKILL_NAME] - assert skills, listing.stdout - assert all(skill["enabled"] is True for skill in skills), skills - assert any(str(skill["source"]).startswith("personal") for skill in skills), skills - - @pytest.mark.managed_fixture @pytest.mark.claude def test_managed_skills_reconcile_lifecycle(live_session, workspace): diff --git a/tests/test_integration_contract.py b/tests/test_integration_contract.py index 9399608f1..7f714c398 100644 --- a/tests/test_integration_contract.py +++ b/tests/test_integration_contract.py @@ -419,7 +419,6 @@ def test_cuj_discovery_fails_for_a_file_without_a_check_name(tmp_path): "antigravity", "managed", "managed-opencode", - "managed-copilot", "dedicated-cuj-plan", "dedicated-cuj", "cujs", From 9f89c7c595290e37f8769a50fcec901a52370776 Mon Sep 17 00:00:00 2001 From: Xi Lyu Date: Fri, 30 Oct 2026 12:42:36 +0000 Subject: [PATCH 05/11] copilot: give VS Code ug's MCP servers and skills without the Copilot CLI Copilot Chat in VS Code reads ~/.copilot/mcp-config.json, which ug now writes, but every MCP and skills command required the copilot binary, so a VS Code-only machine got neither. - A successful `ug configure --agents vscode` records the workspace as set up for VS Code, which makes the copilot MCP client eligible without the Copilot CLI. - `--agents vscode` on `ug mcp add/list/remove` and `ug skills add/remove --via mcp` names that client. Alone it never sets up the copilot agent (which would install the Copilot CLI); it asks for `ug configure --agents vscode` first. - `ug mcp list` reads Copilot's user config when there's no CLI to ask. - A config ug can't parse (comments) and no Copilot CLI now fails with a clear error instead of an uncaught FileNotFoundError, so `ug revert` reports it and carries on. Downloaded skills need nothing new: VS Code reads ~/.agents/skills. Co-authored-by: Isaac --- src/ucode/agents/copilot.py | 13 ++++++++ src/ucode/cli.py | 42 +++++++++++++++++++++----- src/ucode/mcp.py | 35 +++++++++++++++++++--- tests/test_agent_copilot.py | 25 ++++++++++++++++ tests/test_agent_vscode.py | 19 ++++++++++++ tests/test_cli.py | 46 +++++++++++++++++++++++++++++ tests/test_mcp.py | 59 +++++++++++++++++++++++++++++++++++++ 7 files changed, 227 insertions(+), 12 deletions(-) diff --git a/src/ucode/agents/copilot.py b/src/ucode/agents/copilot.py index be2e57353..689055b98 100644 --- a/src/ucode/agents/copilot.py +++ b/src/ucode/agents/copilot.py @@ -29,6 +29,7 @@ import os import re import secrets +import shutil import subprocess import threading import time @@ -821,6 +822,12 @@ def _read_mcp_config_for_rewrite(path: Path) -> dict | None: return data if isinstance(data, dict) else None +def user_mcp_server_names() -> set[str]: + """The servers in Copilot's user config, read without the Copilot CLI.""" + servers = (_read_mcp_config_for_rewrite(copilot_mcp_config_path()) or {}).get("mcpServers") + return set(servers) if isinstance(servers, dict) else set() + + def remove_legacy_mcp_config() -> bool: """Delete the legacy side file and its backup (everything in them is ug's).""" existed = LEGACY_MCP_CONFIG_PATH.exists() or LEGACY_MCP_BACKUP_PATH.exists() @@ -883,6 +890,12 @@ def _apply_mcp_diff(add: dict[str, dict], remove: set[str]) -> set[str]: legacy = _read_legacy_mcp_servers() upserts = {name: entry for name, entry in legacy.items() if name not in remove} | add if config is None: + if not shutil.which(SPEC["binary"]): + raise RuntimeError( + f"{path} isn't plain JSON (it may have comments), and ug edits such a file only " + "through the Copilot CLI. Remove the comments, then try again; VS Code's local " + "agent also skips the file while it has them." + ) # Defer to the copilot CLI, which reads the file the way Copilot does, for exactly the # changed entries. removed = {name for name in remove if _remove_mcp_server_via_cli(name) or name in legacy} diff --git a/src/ucode/cli.py b/src/ucode/cli.py index dc9479f3a..1fd4bfcd7 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -139,6 +139,7 @@ from ucode.mcp import ( MCP_CLIENTS, MCP_ONLY_CLIENTS, + VSCODE_CONFIGURED_KEY, McpServiceListingRateLimited, add_mcp_command, add_skills_command, @@ -149,6 +150,7 @@ configured_mcp_clients, configured_mcp_servers_by_name, list_mcp_command, + mcp_clients_for_agents, purge_cross_workspace_mcp_residue, reconcile_managed_mcp_servers, remove_mcp_command, @@ -1913,7 +1915,8 @@ def _configure_agents_for_mcp( `--profile`) makes that workspace current and skips the workspace prompt; otherwise the prompt is shown on first run. If setup fails, the previous current workspace is restored.""" - scope = {a if a == "cursor" else normalize_tool(a) for a in requested} + names = {a if a in ("cursor", "vscode") else normalize_tool(a) for a in requested} + scope = mcp_clients_for_agents(names) installed = available_mcp_clients() # Setup installs model agents but not MCP-only ones, so a missing MCP-only CLI # would otherwise leave the agent permanently unready and re-enter setup every run. @@ -1922,7 +1925,9 @@ def _configure_agents_for_mcp( raise RuntimeError(_not_installed_message(not_installed)) previous_workspace = load_full_state().get("current_workspace") try: - _bootstrap_agents_for_mcp(scope, installed, workspace_entries) + _bootstrap_agents_for_mcp( + scope, workspace_entries, vscode_only="vscode" in names and "copilot" not in names + ) except BaseException: # Don't leave launches pointed at a workspace whose setup didn't finish. if workspace_entries: @@ -1933,16 +1938,25 @@ def _configure_agents_for_mcp( def _bootstrap_agents_for_mcp( scope: set[str], - installed: list[str], workspace_entries: list[tuple[str, str | None]] | None, + *, + vscode_only: bool = False, ) -> None: workspace_known = True if workspace_entries: url = workspace_entries[0][0] set_current_workspace(url) workspace_known = url in (load_full_state().get("workspaces") or {}) - ready = set(configured_mcp_clients(load_state(), installed)) if workspace_known else set() + state = load_state() + ready = ( + set(configured_mcp_clients(state, available_mcp_clients())) if workspace_known else set() + ) to_bootstrap = scope - ready + if vscode_only and "copilot" in to_bootstrap: + # Setting up the copilot agent here would install the Copilot CLI and its models. + raise RuntimeError( + "VS Code isn't set up for this workspace yet. Run `ug configure --agents vscode` first." + ) model_agents = sorted(a for a in to_bootstrap if a != "cursor") if model_agents: if workspace_entries is None: @@ -2135,7 +2149,10 @@ def mcp_remove( requested_agents = ( None if agents is None - else ({a.strip().lower() for a in agents.split(",") if a.strip()} or None) + else ( + mcp_clients_for_agents({a.strip().lower() for a in agents.split(",") if a.strip()}) + or None + ) ) try: remove_mcp_command(agents=requested_agents) @@ -2168,7 +2185,10 @@ def mcp_list( requested_agents = ( None if agents is None - else ({a.strip().lower() for a in agents.split(",") if a.strip()} or None) + else ( + mcp_clients_for_agents({a.strip().lower() for a in agents.split(",") if a.strip()}) + or None + ) ) try: list_mcp_command(agents=requested_agents) @@ -2429,7 +2449,12 @@ def skills_remove( requested_agents = ( None if agents is None - else ({a.strip().lower() for a in agents.split(",") if a.strip()} or None) + else ( + mcp_clients_for_agents( + {a.strip().lower() for a in agents.split(",") if a.strip()} + ) + or None + ) ) locations = _parse_skill_locations(location) if locations: @@ -4753,7 +4778,8 @@ def configure( state = load_state() managed, _ = _read_policy_for_other_agent(state) refuse_user_scope_over_managed_config(state.get("workspace"), managed) - vscode.configure(state, provider=provider, location=model_location) + if vscode.configure(state, provider=provider, location=model_location): + save_state({**state, VSCODE_CONFIGURED_KEY: True}) 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). diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index a244a1357..7fe591b1c 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -130,6 +130,9 @@ # MCP-only clients ucode never launches for model routing, so they never land in # `available_tools`; they're eligible for MCP config purely on being installed. MCP_ONLY_CLIENTS = ("cursor",) +# Copilot Chat in VS Code reads Copilot's user MCP config, so a workspace whose state carries this +# key gets the `copilot` client even without the Copilot CLI, and `--agents vscode` names it. +VSCODE_CONFIGURED_KEY = "vscode_configured" EXTERNAL_MCP_SELECTION_PREFIX = "external:" SQL_MCP_VALUE = "managed:sql" GENIE_SPACE_SELECTION_PREFIX = "genie-space:" @@ -258,11 +261,32 @@ def available_mcp_clients() -> list[str]: return [client for client, spec in MCP_CLIENTS.items() if shutil.which(str(spec["binary"]))] +def _with_vscode_copilot(state: dict, clients: list[str]) -> list[str]: + """``clients`` plus ``copilot`` when VS Code is set up for this workspace.""" + if not state.get(VSCODE_CONFIGURED_KEY): + return clients + return [client for client in MCP_CLIENTS if client in clients or client == "copilot"] + + +def mcp_clients_for_agents(agents: set[str]) -> set[str]: + """``--agents`` names as MCP clients.""" + return {"copilot" if agent == "vscode" else agent for agent in agents} + + +def _client_display(client: str) -> str: + if client == "copilot" and not shutil.which(str(MCP_CLIENTS[client]["binary"])): + return "GitHub Copilot in VS Code" + return str(MCP_CLIENTS[client]["display"]) + + def configured_mcp_clients(state: dict, installed_clients: list[str]) -> list[str]: configured_tools = state.get("available_tools") or [] if not isinstance(configured_tools, list): configured_tools = [] configured = set(configured_tools) + if state.get(VSCODE_CONFIGURED_KEY): + configured.add("copilot") + installed_clients = _with_vscode_copilot(state, installed_clients) return [ client for client in MCP_CLIENTS @@ -1697,7 +1721,7 @@ def remove_for(client: str, names: set[str]) -> None: def purge_cross_workspace_mcp_residue(state: dict, workspace: str) -> None: - installed = set(available_mcp_clients()) + installed = set(_with_vscode_copilot(state, available_mcp_clients())) # Collect every removal as {client: {names}} and apply it as one batched write per client below, # rather than a CLI/config removal per server (see `_batch_remove_stale_servers`). batch_remove: dict[str, set[str]] = {} @@ -1908,7 +1932,7 @@ def setup_mcp_clients( purge_cross_workspace_mcp_residue(state, workspace) - installed_clients = available_mcp_clients() + installed_clients = _with_vscode_copilot(state, available_mcp_clients()) if not installed_clients: raise RuntimeError( "No supported MCP clients are installed. Install Claude, Codex, Gemini, OpenCode, " @@ -1940,7 +1964,7 @@ def setup_mcp_clients( if not quiet: print_section(section) - client_names = ", ".join(str(MCP_CLIENTS[client]["display"]) for client in clients) + client_names = ", ".join(_client_display(client) for client in clients) print_note(f"{action_note}: {client_names}") for client in missing_clients: print_warning( @@ -2592,6 +2616,9 @@ def _run_mcp_list(client: str) -> str | None: def query_live_mcp_status(client: str) -> dict[str, str]: """Live ``{server_name: state}`` for one installed agent (empty if its listing can't be read).""" + if client == "copilot" and not shutil.which(str(MCP_CLIENTS[client]["binary"])): + # VS Code only: no Copilot CLI to ask, so report what Copilot's user config holds. + return dict.fromkeys(copilot.user_mcp_server_names(), LIVE_ENABLED) output = _run_mcp_list(client) if output is None: return {} @@ -2767,7 +2794,7 @@ def list_mcp_command(agents: set[str] | None = None) -> int: ) state = load_state() - installed = available_mcp_clients() + installed = _with_vscode_copilot(state, available_mcp_clients()) probe_clients = [client for client in installed if agents is None or client in agents] scope_note = "" if agents is None else f" for {', '.join(sorted(agents))}" diff --git a/tests/test_agent_copilot.py b/tests/test_agent_copilot.py index f416b1ad2..f27251963 100644 --- a/tests/test_agent_copilot.py +++ b/tests/test_agent_copilot.py @@ -925,6 +925,7 @@ def run(argv, **kwargs): return subprocess.CompletedProcess(argv, 1 if stderr else 0, "", stderr) monkeypatch.setattr(copilot.subprocess_cross_os, "run", run) + monkeypatch.setattr(copilot.shutil, "which", lambda binary: f"/usr/bin/{binary}") return calls def test_defers_to_the_copilot_cli_for_a_config_it_cannot_parse(self, monkeypatch): @@ -948,6 +949,30 @@ def test_defers_to_the_copilot_cli_for_a_config_it_cannot_parse(self, monkeypatc assert path.read_text(encoding="utf-8") == jsonc assert not copilot.LEGACY_MCP_CONFIG_PATH.exists() + def test_commented_config_without_the_copilot_cli_is_left_alone(self, monkeypatch): + # VS Code alone has no Copilot CLI to edit a file ug can't parse. + calls = self._stub_cli(monkeypatch) + monkeypatch.setattr(copilot.shutil, "which", lambda binary: None) + path = copilot.copilot_mcp_config_path() + path.parent.mkdir(parents=True, exist_ok=True) + jsonc = '{\n // mine\n "mcpServers": {"mine": {"x": 1}},\n}\n' + path.write_text(jsonc, encoding="utf-8") + + with pytest.raises(RuntimeError, match="Remove the comments"): + copilot.write_user_mcp_servers({"svc": self.ENTRY}, set()) + + assert calls == [] + assert path.read_text(encoding="utf-8") == jsonc + + def test_user_mcp_server_names_reads_the_config_without_the_cli(self): + path = copilot.copilot_mcp_config_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps({"mcpServers": {"svc": self.ENTRY}}), encoding="utf-8") + assert copilot.user_mcp_server_names() == {"svc"} + + path.write_text('{\n // mine\n "mcpServers": {"svc": {}}\n}\n', encoding="utf-8") + assert copilot.user_mcp_server_names() == set() + def test_failed_cli_add_keeps_the_legacy_side_file(self, monkeypatch): self._stub_cli(monkeypatch, fail_add=True) path = copilot.copilot_mcp_config_path() diff --git a/tests/test_agent_vscode.py b/tests/test_agent_vscode.py index 27deea8cd..dfb90e8cf 100644 --- a/tests/test_agent_vscode.py +++ b/tests/test_agent_vscode.py @@ -16,6 +16,7 @@ from ucode.cli import app from ucode.databricks import AnthropicModelCatalog from ucode.managed_files import OS +from ucode.mcp import VSCODE_CONFIGURED_KEY # The conftest fixture stubs these out for every test; the ones here exercise the real lookups. REAL_FIND_CODE = vscode.find_code @@ -511,6 +512,24 @@ def test_configure_agents_vscode_sets_up_the_workspace_then_vscode(monkeypatch, assert ("VS Code isn't changed" in result.output) == dry_run +@pytest.mark.parametrize("installed", [True, False]) +def test_configure_agents_vscode_records_vscode_for_mcp_only_when_it_installs( + monkeypatch, installed +): + saved: list[dict] = [] + monkeypatch.setattr(config_io, "_dry_run", False) + monkeypatch.setattr("ucode.cli._configure_shared_workspace_states", lambda *a, **k: []) + monkeypatch.setattr("ucode.cli.load_state", lambda: STATE) + monkeypatch.setattr("ucode.cli.save_state", saved.append) + monkeypatch.setattr(vscode, "configure", lambda state, **_: installed) + + result = CliRunner().invoke(app, ["configure", "--agents", "vscode", "--workspace", WS]) + + assert result.exit_code == 0, result.output + assert saved == ([{**STATE, VSCODE_CONFIGURED_KEY: True}] if installed else []) + assert VSCODE_CONFIGURED_KEY not in STATE + + @pytest.mark.parametrize( ("args", "error"), [ diff --git a/tests/test_cli.py b/tests/test_cli.py index f460fc67b..d1f666fdc 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2445,6 +2445,17 @@ def test_mcp_list_forwards_agents_option(self, monkeypatch): assert result.exit_code == 0, result.output assert calls == [{"claude", "codex"}] + @pytest.mark.parametrize("command", ["list", "remove"]) + def test_mcp_agents_vscode_means_copilot_config(self, monkeypatch, command): + calls: list[set[str] | None] = [] + monkeypatch.setattr( + cli_mod, f"{command}_mcp_command", lambda agents=None: calls.append(agents) + ) + monkeypatch.setattr(cli_mod, "_sync_claude_cowork_plugin", lambda: None) + result = runner.invoke(app, ["mcp", command, "--agents", "vscode,claude"]) + assert result.exit_code == 0, result.output + assert calls == [{"copilot", "claude"}] + def test_mcp_add_syncs_claude_cowork(self): with ( patch("ucode.cli.add_mcp_command") as mock_add, @@ -3289,6 +3300,41 @@ def test_agent_still_unready_after_setup_errors(self): ): cli_mod._configure_agents_for_mcp(["codex"]) + def test_vscode_targets_copilot_config_without_the_copilot_cli(self): + state = {"workspace": "https://ws", cli_mod.VSCODE_CONFIGURED_KEY: True} + with ( + patch("ucode.cli.load_state", return_value=state), + patch("ucode.cli.available_mcp_clients", return_value=["claude"]), + patch("ucode.cli.configure_workspace_command") as mock_cfg, + ): + scope = cli_mod._configure_agents_for_mcp(["vscode"]) + + assert scope == {"copilot"} + mock_cfg.assert_not_called() + + def test_vscode_not_set_up_errors_instead_of_installing_copilot(self): + with ( + patch("ucode.cli.load_state", return_value={"workspace": "https://ws"}), + patch("ucode.cli.available_mcp_clients", return_value=[]), + patch("ucode.cli.configure_workspace_command") as mock_cfg, + pytest.raises(RuntimeError, match="ug configure --agents vscode"), + ): + cli_mod._configure_agents_for_mcp(["vscode"]) + + mock_cfg.assert_not_called() + + def test_vscode_with_copilot_sets_up_the_copilot_cli(self): + with ( + patch("ucode.cli.load_state", return_value={"workspace": "https://ws"}), + patch("ucode.cli.available_mcp_clients", return_value=["copilot"]), + patch("ucode.cli.configured_mcp_clients", side_effect=[[], ["copilot"]]), + patch("ucode.cli.configure_workspace_command") as mock_cfg, + ): + scope = cli_mod._configure_agents_for_mcp(["vscode", "copilot"]) + + assert scope == {"copilot"} + mock_cfg.assert_called_once_with(selected_tools=["copilot"]) + class TestMcpAddWorkspace: def test_workspace_flag_forwarded_to_agent_setup(self): diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 6019ce09a..3f77fb5ec 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -155,6 +155,37 @@ def test_skipped_when_binary_not_installed(self): assert "cursor" not in clients +class TestVscodeMcpClient: + STATE = {"workspace": WS, "available_tools": [], mcp.VSCODE_CONFIGURED_KEY: True} + + def test_vscode_setup_makes_copilot_eligible_without_its_cli(self): + assert mcp.configured_mcp_clients(self.STATE, ["claude"]) == ["copilot"] + + def test_copilot_still_needs_its_cli_without_a_vscode_setup(self): + assert mcp.configured_mcp_clients({"available_tools": ["copilot"]}, ["claude"]) == [] + + def test_agents_vscode_names_the_copilot_client(self): + assert mcp.mcp_clients_for_agents({"vscode", "claude"}) == {"copilot", "claude"} + + def test_setup_targets_copilot_in_vscode_when_no_agent_cli_is_installed( + self, monkeypatch, capsys + ): + monkeypatch.setattr(mcp.shutil, "which", lambda binary: None) + + _, _, clients = mcp.setup_mcp_clients(self.STATE, "MCP", require_auth=False) + + assert clients == ["copilot"] + assert "GitHub Copilot in VS Code" in _unwrap(capsys.readouterr().out) + + def test_list_reads_copilot_config_when_its_cli_is_missing(self, monkeypatch): + monkeypatch.setattr(mcp.shutil, "which", lambda binary: None) + path = mcp.copilot.copilot_mcp_config_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps({"mcpServers": {"svc": {"type": "local"}}}), encoding="utf-8") + + assert mcp.query_live_mcp_status("copilot") == {"svc": mcp.LIVE_ENABLED} + + class TestKiloMcpClient: def test_kilo_registered_in_mcp_clients(self): assert "kilo" in mcp.MCP_CLIENTS @@ -3141,6 +3172,34 @@ def test_removes_copilot_servers_from_its_user_config_and_the_legacy_side_file(s assert json.loads(user_config.read_text())["mcpServers"] == {"mine": {"x": 1}} assert not legacy.exists() + def test_commented_copilot_config_without_its_cli_does_not_stop_the_revert( + self, monkeypatch, capsys + ): + monkeypatch.setattr(mcp.copilot.shutil, "which", lambda binary: None) + user_config = mcp.copilot.copilot_mcp_config_path() + user_config.parent.mkdir(parents=True, exist_ok=True) + jsonc = '{\n // mine\n "mcpServers": {"svc": {"type": "local"}}\n}\n' + user_config.write_text(jsonc, encoding="utf-8") + removed: list[tuple[str, str]] = [] + real_remove = mcp.remove_client_mcp_server + + def remove(client, name): + if client == "copilot": + return real_remove(client, name) + removed.append((client, name)) + return ["user"] + + monkeypatch.setattr(mcp, "remove_client_mcp_server", remove) + + result = mcp.revert_mcp_configs( + {"mcp_servers": [{"name": "svc", "clients": ["copilot", "claude"]}]} + ) + + assert removed == [("claude", "svc")] + assert result == {"claude": True} + assert user_config.read_text(encoding="utf-8") == jsonc + assert "Remove the comments" in _unwrap(capsys.readouterr().out) + def test_removes_skills_registry_across_its_clients(self, monkeypatch): removed: list[tuple[str, str]] = [] monkeypatch.setattr( From 9c5a5aaecb5981498562d4e72ea804d3a3ef4e65 Mon Sep 17 00:00:00 2001 From: Xi Lyu Date: Fri, 30 Oct 2026 13:59:38 +0000 Subject: [PATCH 06/11] copilot: use AGENT_COPILOT for the VS Code MCP checks in cli.py Co-authored-by: Isaac --- src/ucode/cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 1fd4bfcd7..8d7a1622d 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -1926,7 +1926,7 @@ def _configure_agents_for_mcp( previous_workspace = load_full_state().get("current_workspace") try: _bootstrap_agents_for_mcp( - scope, workspace_entries, vscode_only="vscode" in names and "copilot" not in names + scope, workspace_entries, vscode_only="vscode" in names and AGENT_COPILOT not in names ) except BaseException: # Don't leave launches pointed at a workspace whose setup didn't finish. @@ -1952,7 +1952,7 @@ def _bootstrap_agents_for_mcp( set(configured_mcp_clients(state, available_mcp_clients())) if workspace_known else set() ) to_bootstrap = scope - ready - if vscode_only and "copilot" in to_bootstrap: + if vscode_only and AGENT_COPILOT in to_bootstrap: # Setting up the copilot agent here would install the Copilot CLI and its models. raise RuntimeError( "VS Code isn't set up for this workspace yet. Run `ug configure --agents vscode` first." From 000272b05806e16a6b4974bd90604aba8f0f2f89 Mon Sep 17 00:00:00 2001 From: Xi Lyu Date: Fri, 30 Oct 2026 14:01:22 +0000 Subject: [PATCH 07/11] copilot: add AGENT_CURSOR and AGENT_VSCODE for the MCP agent checks Co-authored-by: Isaac --- src/ucode/cli.py | 8 ++++++-- src/ucode/constants.py | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 8d7a1622d..9b365c269 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -57,6 +57,8 @@ AGENT_CLAUDE, AGENT_CODEX, AGENT_COPILOT, + AGENT_CURSOR, + AGENT_VSCODE, ISAAC_SMART_ROUTER_FEEDBACK_ENV_VAR, SMART_ROUTING_ENV_KEYS, UG_HEADERS_ENV_VAR, @@ -1915,7 +1917,7 @@ def _configure_agents_for_mcp( `--profile`) makes that workspace current and skips the workspace prompt; otherwise the prompt is shown on first run. If setup fails, the previous current workspace is restored.""" - names = {a if a in ("cursor", "vscode") else normalize_tool(a) for a in requested} + names = {a if a in (AGENT_CURSOR, AGENT_VSCODE) else normalize_tool(a) for a in requested} scope = mcp_clients_for_agents(names) installed = available_mcp_clients() # Setup installs model agents but not MCP-only ones, so a missing MCP-only CLI @@ -1926,7 +1928,9 @@ def _configure_agents_for_mcp( previous_workspace = load_full_state().get("current_workspace") try: _bootstrap_agents_for_mcp( - scope, workspace_entries, vscode_only="vscode" in names and AGENT_COPILOT not in names + scope, + workspace_entries, + vscode_only=AGENT_VSCODE in names and AGENT_COPILOT not in names, ) except BaseException: # Don't leave launches pointed at a workspace whose setup didn't finish. diff --git a/src/ucode/constants.py b/src/ucode/constants.py index 8ded72215..ad4d054af 100644 --- a/src/ucode/constants.py +++ b/src/ucode/constants.py @@ -8,6 +8,8 @@ AGENT_CLAUDE = "claude" AGENT_CODEX = "codex" AGENT_COPILOT = "copilot" +AGENT_CURSOR = "cursor" +AGENT_VSCODE = "vscode" ENABLE_SMART_ROUTING_ENV_VAR = "ENABLE_SMART_ROUTING_V2" ENABLE_SUBAGENT_ROUTING_ENV_VAR = "ENABLE_SMART_ROUTING_SUBAGENT_ONLY" From e02a8d1a4a7b17a42c68964a94f7b9a6c9df6928 Mon Sep 17 00:00:00 2001 From: Xi Lyu Date: Fri, 30 Oct 2026 14:17:28 +0000 Subject: [PATCH 08/11] copilot: label VS Code-only setups in mcp list and revert; drop an emptied Copilot MCP config For a workspace set up for VS Code with no Copilot CLI, ug mcp list's note, ug revert's summary row and its removal warning now say "GitHub Copilot in VS Code" instead of "GitHub Copilot CLI". The label needs the VS Code marker, so a machine that simply lacks the Copilot CLI keeps the old name. When a removal leaves Copilot's user config with nothing but an empty mcpServers, ug deletes the file instead of leaving {"mcpServers": {}} behind. Copilot reads a missing file the same way; a config with any other key is rewritten as before. Co-authored-by: Isaac --- src/ucode/agents/copilot.py | 10 +++++++++- src/ucode/cli.py | 5 +++-- src/ucode/mcp.py | 16 +++++++++++----- tests/test_agent_copilot.py | 18 ++++++++++++++++++ tests/test_mcp.py | 15 +++++++++++++++ 5 files changed, 56 insertions(+), 8 deletions(-) diff --git a/src/ucode/agents/copilot.py b/src/ucode/agents/copilot.py index 689055b98..4d34efe50 100644 --- a/src/ucode/agents/copilot.py +++ b/src/ucode/agents/copilot.py @@ -913,7 +913,15 @@ def _apply_mcp_diff(add: dict[str, dict], remove: set[str]) -> set[str]: servers.pop(name, None) servers.update(upserts) config["mcpServers"] = servers - write_json_file(path, config) + if config == {"mcpServers": {}}: + # Copilot reads a missing file the same way, so don't leave an empty one behind. + if not is_dry_run(): + try: + path.unlink(missing_ok=True) + except OSError as exc: + raise RuntimeError(f"Failed to remove {path}") from exc + else: + write_json_file(path, config) remove_legacy_mcp_config() return removed diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 9b365c269..1e48ae202 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -152,6 +152,7 @@ configured_mcp_clients, configured_mcp_servers_by_name, list_mcp_command, + mcp_client_display, mcp_clients_for_agents, purge_cross_workspace_mcp_residue, reconcile_managed_mcp_servers, @@ -1626,9 +1627,9 @@ def revert() -> int: print_kv("Unity Gateway workspace policy", workspace_policy_result) print_kv("Pi settings", "restored" if pi_settings_restored else "unchanged") print_kv("VS Code extension", vscode_result) - for client, spec in MCP_CLIENTS.items(): + for client in MCP_CLIENTS: print_kv( - f"{spec['display']} MCP config", + f"{mcp_client_display(client, state)} MCP config", "restored" if mcp_results.get(client) else "unchanged", ) if desktop_result == "failed": diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 7fe591b1c..ad66a2059 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -273,8 +273,12 @@ def mcp_clients_for_agents(agents: set[str]) -> set[str]: return {"copilot" if agent == "vscode" else agent for agent in agents} -def _client_display(client: str) -> str: - if client == "copilot" and not shutil.which(str(MCP_CLIENTS[client]["binary"])): +def mcp_client_display(client: str, state: dict) -> str: + if ( + client == "copilot" + and state.get(VSCODE_CONFIGURED_KEY) + and not shutil.which(str(MCP_CLIENTS[client]["binary"])) + ): return "GitHub Copilot in VS Code" return str(MCP_CLIENTS[client]["display"]) @@ -497,7 +501,7 @@ def revert_mcp_configs(state: dict) -> dict[str, bool]: try: removed_scopes = remove_client_mcp_server(client, name) except RuntimeError as exc: - display = MCP_CLIENTS[client]["display"] + display = mcp_client_display(client, state) print_warning(f"Could not remove MCP server `{name}` from {display}: {exc}") continue results[client] = bool(removed_scopes) or results.get(client, False) @@ -1964,7 +1968,7 @@ def setup_mcp_clients( if not quiet: print_section(section) - client_names = ", ".join(_client_display(client) for client in clients) + client_names = ", ".join(mcp_client_display(client, state) for client in clients) print_note(f"{action_note}: {client_names}") for client in missing_clients: print_warning( @@ -2848,7 +2852,9 @@ def list_mcp_command(agents: set[str] | None = None) -> int: live_note = "Status is from each agent's `mcp list`" # Only mention the enabled/disabled caveat for the agents actually in the reported set. config_state_clients = [ - MCP_CLIENTS[client]["display"] for client in ("codex", "copilot") if client in probe_clients + mcp_client_display(client, state) + for client in ("codex", "copilot") + if client in probe_clients ] if config_state_clients: verb = "reports" if len(config_state_clients) == 1 else "report" diff --git a/tests/test_agent_copilot.py b/tests/test_agent_copilot.py index f27251963..0315d088d 100644 --- a/tests/test_agent_copilot.py +++ b/tests/test_agent_copilot.py @@ -973,6 +973,24 @@ def test_user_mcp_server_names_reads_the_config_without_the_cli(self): path.write_text('{\n // mine\n "mcpServers": {"svc": {}}\n}\n', encoding="utf-8") assert copilot.user_mcp_server_names() == set() + def test_removing_the_last_server_deletes_a_config_with_nothing_else(self): + path = copilot.copilot_mcp_config_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps({"mcpServers": {"svc": self.ENTRY}}), encoding="utf-8") + + assert copilot.write_user_mcp_servers({}, {"svc"}) == {"svc"} + + assert not path.exists() + + def test_removing_the_last_server_keeps_a_config_with_other_keys(self): + path = copilot.copilot_mcp_config_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps({"mcpServers": {"svc": self.ENTRY}, "x": 1}), encoding="utf-8") + + copilot.write_user_mcp_servers({}, {"svc"}) + + assert json.loads(path.read_text(encoding="utf-8")) == {"mcpServers": {}, "x": 1} + def test_failed_cli_add_keeps_the_legacy_side_file(self, monkeypatch): self._stub_cli(monkeypatch, fail_add=True) path = copilot.copilot_mcp_config_path() diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 3f77fb5ec..1b64b67a9 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -185,6 +185,21 @@ def test_list_reads_copilot_config_when_its_cli_is_missing(self, monkeypatch): assert mcp.query_live_mcp_status("copilot") == {"svc": mcp.LIVE_ENABLED} + @pytest.mark.parametrize( + ("vscode", "cli", "label"), + [ + (True, None, "GitHub Copilot in VS Code"), + (True, "/usr/bin/copilot", "GitHub Copilot CLI"), + (False, None, "GitHub Copilot CLI"), + ], + ) + def test_copilot_is_labeled_vscode_only_for_a_vscode_setup_without_the_cli( + self, monkeypatch, vscode, cli, label + ): + monkeypatch.setattr(mcp.shutil, "which", lambda binary: cli) + state = {mcp.VSCODE_CONFIGURED_KEY: True} if vscode else {} + assert mcp.mcp_client_display("copilot", state) == label + class TestKiloMcpClient: def test_kilo_registered_in_mcp_clients(self): From 48cf89c96fb5120878b40b2178d87d0f9a771b91 Mon Sep 17 00:00:00 2001 From: Xi Lyu Date: Fri, 30 Oct 2026 14:19:44 +0000 Subject: [PATCH 09/11] copilot: use AGENT_COPILOT and AGENT_VSCODE for agent ids in cli.py and mcp.py Command names (the copilot subcommand and the help order) and the copilot binary name stay string literals. Co-authored-by: Isaac --- src/ucode/cli.py | 40 ++++++++++++++++++++++------------------ src/ucode/mcp.py | 28 ++++++++++++++-------------- 2 files changed, 36 insertions(+), 32 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 1e48ae202..6899ae4d7 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -231,8 +231,8 @@ CustomOAuthConfig = custom_oauth.CustomOAuthConfig _DISCOVERY_CONSUMERS: dict[str, tuple[str, ...]] = { - "claude": ("claude", "opencode", "kilo", "copilot", "pi"), - "codex": ("codex", "copilot", "pi"), + "claude": ("claude", "opencode", "kilo", AGENT_COPILOT, "pi"), + "codex": ("codex", AGENT_COPILOT, "pi"), "gemini": ("gemini", "opencode", "kilo", "pi", "antigravity"), "oss": ("opencode", "kilo"), } @@ -676,7 +676,7 @@ def configure_shared_state( or "claude" in tools or "opencode" in tools or "kilo" in tools - or "copilot" in tools + or AGENT_COPILOT in tools or "pi" in tools ) want_gemini = ( @@ -689,12 +689,16 @@ def configure_shared_state( ) # Claude's web-search server also needs a Responses-capable model. want_codex = ( - fetch_all or "codex" in tools or "claude" in tools or "copilot" in tools or "pi" in tools + fetch_all + or "codex" in tools + or "claude" in tools + or AGENT_COPILOT in tools + or "pi" in tools ) # Codex smart routing can select OSS models such as GLM, so a Codex-only # configure must persist that discovered family too. want_oss = fetch_all or "opencode" in tools or "kilo" in tools or "codex" in tools - want_copilot = fetch_all or "copilot" in tools + want_copilot = fetch_all or AGENT_COPILOT in tools claude_reason: str | None = None gemini_reason: str | None = None @@ -792,7 +796,7 @@ def configure_shared_state( "gemini": gemini_reason, "codex": codex_reason, "oss": oss_reason, - "copilot": copilot_reason, + AGENT_COPILOT: copilot_reason, } return state @@ -997,7 +1001,7 @@ def _maybe_select_provider_service(tool: str, state: dict) -> dict: back to Databricks when no matching provider services are found or the listing fails and no custom location is offered. """ - if tool not in ("claude", "codex", "gemini", "antigravity", "copilot"): + if tool not in ("claude", "codex", "gemini", "antigravity", AGENT_COPILOT): return state display = TOOL_SPECS[tool]["display"] @@ -1354,7 +1358,7 @@ def _model_values(value: object) -> list[str]: def _status_models(tool: str, state: dict) -> list[str]: """Return the effective model allow-list for one configured agent.""" static_models = _model_values(state.get(f"{tool}_static_models")) - if tool == "copilot": + if tool == AGENT_COPILOT: models = [model.id for model in copilot_agent.copilot_catalog(state)] elif static_models: models = static_models @@ -1377,7 +1381,7 @@ def _status_models(tool: str, state: dict) -> list[str]: def _status_default_model(tool: str, state: dict, models: list[str]) -> str | None: - if tool == "copilot": + if tool == AGENT_COPILOT: return copilot_agent.default_model(state) explicit = state.get(f"{tool}_default_model") if isinstance(explicit, str) and explicit: @@ -1433,7 +1437,7 @@ def _live_status_model_state(state: dict, tools: set[str]) -> tuple[dict, str]: if oss_models: opencode_models["oss"] = oss_models live["opencode_models"] = opencode_models - if "copilot" in tools: + if AGENT_COPILOT in tools: copilot_models = copilot_agent.catalog_from_listings( cached_model_service_entries(workspace), [*claude_models.values(), *codex_models], @@ -3299,7 +3303,7 @@ def _child_owns_stdout(tool: str, tool_args: list[str]) -> bool: tool_args = tool_args[: tool_args.index("--")] if tool == "claude": return any(arg in {"-p", "--print"} for arg in tool_args) - if tool == "copilot": + if tool == AGENT_COPILOT: return any(arg in {"-p", "--prompt"} or arg.startswith("--prompt=") for arg in tool_args) return tool == "codex" and any(arg in {"exec", "e", "app-server"} for arg in tool_args) @@ -3379,7 +3383,7 @@ def _skip_model_discovery( (``family_defaults``). In the last case it is also harmful: discovered family defaults would overwrite that writer's on every launch, and each rewrite of the managed file costs a sudo prompt. """ - if tool == "copilot": + if tool == AGENT_COPILOT: # Copilot registers its discovered catalog, so a managed default alone still needs it. return bool( provider or managed_parent_schema or (managed and managed_static_models(managed, tool)) @@ -3904,7 +3908,7 @@ def _launch_tool( provider=provider, # Copilot picks its start model from its own catalog; the admin's or budget pick leads. # session_model is set for claude only. - launch_model=managed_model if tool == "copilot" else session_model, + launch_model=managed_model if tool == AGENT_COPILOT else session_model, ) print_success(f"Starting {TOOL_SPECS[tool]['display']}") with _smart_routing_v2_flag(True if managed_smart_routing_applies else None, agent=tool): @@ -4378,7 +4382,7 @@ def copilot_cmd( ) -> None: """Launch GitHub Copilot CLI via Databricks.""" _launch_tool( - "copilot", + AGENT_COPILOT, ctx, provider=provider, skip_preflight=skip_preflight, @@ -4677,8 +4681,8 @@ def configure( requested_agents = [a.strip().lower() for a in (agents or "").split(",") if a.strip()] # Both flags choose VS Code's models alone, so another model agent in the same run # would silently keep the workspace's own. Cursor has no models to choose. - if "vscode" not in requested_agents or [ - a for a in requested_agents if a not in ("vscode", "cursor") + if AGENT_VSCODE not in requested_agents or [ + a for a in requested_agents if a not in (AGENT_VSCODE, "cursor") ]: raise RuntimeError( "--provider and --model-location work only with `--agents vscode`. " @@ -4746,8 +4750,8 @@ def configure( # VS Code is configure-only too: it needs the workspace, then its own setup below. requested = [a.strip().lower() for a in agents.split(",") if a.strip()] wants_cursor = "cursor" in requested - wants_vscode = "vscode" in requested - model_agent_names = ",".join(a for a in requested if a not in ("cursor", "vscode")) + wants_vscode = AGENT_VSCODE in requested + model_agent_names = ",".join(a for a in requested if a not in ("cursor", AGENT_VSCODE)) if model_agent_names: selected_tools = _parse_agents_option(model_agent_names) if workspace_entries is None: diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index ad66a2059..3166e8ee3 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -18,7 +18,7 @@ from ucode.agents import antigravity, claude, codex, copilot, cursor, gemini, kilo, opencode from ucode.config_io import read_json_safe, read_toml_safe -from ucode.constants import MCP_CLEANUP_SCOPES, MCP_USER_SCOPE +from ucode.constants import AGENT_COPILOT, AGENT_VSCODE, MCP_CLEANUP_SCOPES, MCP_USER_SCOPE from ucode.databricks import ( AIGW_MCP_SERVICES_SEGMENT, AuthTokenError, @@ -108,7 +108,7 @@ "display": "Kilo", "list_command": "kilo mcp list", }, - "copilot": { + AGENT_COPILOT: { "binary": "copilot", "display": "GitHub Copilot CLI", "list_command": "copilot mcp list --json", @@ -265,17 +265,17 @@ def _with_vscode_copilot(state: dict, clients: list[str]) -> list[str]: """``clients`` plus ``copilot`` when VS Code is set up for this workspace.""" if not state.get(VSCODE_CONFIGURED_KEY): return clients - return [client for client in MCP_CLIENTS if client in clients or client == "copilot"] + return [client for client in MCP_CLIENTS if client in clients or client == AGENT_COPILOT] def mcp_clients_for_agents(agents: set[str]) -> set[str]: """``--agents`` names as MCP clients.""" - return {"copilot" if agent == "vscode" else agent for agent in agents} + return {AGENT_COPILOT if agent == AGENT_VSCODE else agent for agent in agents} def mcp_client_display(client: str, state: dict) -> str: if ( - client == "copilot" + client == AGENT_COPILOT and state.get(VSCODE_CONFIGURED_KEY) and not shutil.which(str(MCP_CLIENTS[client]["binary"])) ): @@ -289,7 +289,7 @@ def configured_mcp_clients(state: dict, installed_clients: list[str]) -> list[st configured_tools = [] configured = set(configured_tools) if state.get(VSCODE_CONFIGURED_KEY): - configured.add("copilot") + configured.add(AGENT_COPILOT) installed_clients = _with_vscode_copilot(state, installed_clients) return [ client @@ -444,7 +444,7 @@ def configure_client_mcp_server( if client == "kilo": removed = kilo.write_mcp_server_config(name, argv) return [MCP_USER_SCOPE] if removed else [] - if client == "copilot": + if client == AGENT_COPILOT: removed = copilot.write_mcp_server_config(name, argv) return [MCP_USER_SCOPE] if removed else [] if client == "cursor": @@ -469,7 +469,7 @@ def remove_client_mcp_server(client: str, name: str) -> list[str]: return [MCP_USER_SCOPE] if opencode.remove_mcp_server_config(name) else [] if client == "kilo": return [MCP_USER_SCOPE] if kilo.remove_mcp_server_config(name) else [] - if client == "copilot": + if client == AGENT_COPILOT: return [MCP_USER_SCOPE] if copilot.remove_mcp_server_config(name) else [] if client == "cursor": return [MCP_USER_SCOPE] if cursor.remove_mcp_server_config(name) else [] @@ -488,7 +488,7 @@ def revert_mcp_configs(state: dict) -> dict[str, bool]: # One agent's unreadable config must not stop the revert of the others. try: if copilot.remove_legacy_mcp_config(): - results["copilot"] = True + results[AGENT_COPILOT] = True except RuntimeError as exc: print_warning(f"Could not remove ug's old Copilot MCP file: {exc}") for server in all_servers: @@ -1476,7 +1476,7 @@ def _discover_selected_mcp_sources( "claude": claude, "codex": codex, "gemini": gemini, - "copilot": copilot, + AGENT_COPILOT: copilot, "cursor": cursor, "opencode": opencode, "kilo": kilo, @@ -1520,7 +1520,7 @@ def _managed_mcp_entry( return cursor.build_mcp_server_entry(argv) if client == "gemini": return gemini.build_mcp_server_entry(argv) - if client == "copilot": + if client == AGENT_COPILOT: return copilot.build_mcp_server_entry(argv) if client == "opencode": return opencode.build_mcp_server_entry(argv) @@ -2574,7 +2574,7 @@ def parse_mcp_list_output(client: str, output: str) -> dict[str, str]: if client in ("codex", "antigravity"): # Both print a columnar table with an enabled/disabled state column. parsed = _parse_codex_mcp_list(output) - elif client == "copilot": + elif client == AGENT_COPILOT: parsed = _parse_copilot_mcp_list(output) elif client in ("kilo", "opencode"): parsed = _parse_tree_mcp_list(output) @@ -2620,7 +2620,7 @@ def _run_mcp_list(client: str) -> str | None: def query_live_mcp_status(client: str) -> dict[str, str]: """Live ``{server_name: state}`` for one installed agent (empty if its listing can't be read).""" - if client == "copilot" and not shutil.which(str(MCP_CLIENTS[client]["binary"])): + if client == AGENT_COPILOT and not shutil.which(str(MCP_CLIENTS[client]["binary"])): # VS Code only: no Copilot CLI to ask, so report what Copilot's user config holds. return dict.fromkeys(copilot.user_mcp_server_names(), LIVE_ENABLED) output = _run_mcp_list(client) @@ -2853,7 +2853,7 @@ def list_mcp_command(agents: set[str] | None = None) -> int: # Only mention the enabled/disabled caveat for the agents actually in the reported set. config_state_clients = [ mcp_client_display(client, state) - for client in ("codex", "copilot") + for client in ("codex", AGENT_COPILOT) if client in probe_clients ] if config_state_clients: From 54f27fbbcb29f9826dfc3839c950a1abcbf937f7 Mon Sep 17 00:00:00 2001 From: Xi Lyu Date: Fri, 30 Oct 2026 14:21:43 +0000 Subject: [PATCH 10/11] Revert "copilot: use AGENT_COPILOT and AGENT_VSCODE for agent ids in cli.py and mcp.py" This reverts commit 48cf89c96fb5120878b40b2178d87d0f9a771b91. Co-authored-by: Isaac --- src/ucode/cli.py | 40 ++++++++++++++++++---------------------- src/ucode/mcp.py | 28 ++++++++++++++-------------- 2 files changed, 32 insertions(+), 36 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 6899ae4d7..1e48ae202 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -231,8 +231,8 @@ CustomOAuthConfig = custom_oauth.CustomOAuthConfig _DISCOVERY_CONSUMERS: dict[str, tuple[str, ...]] = { - "claude": ("claude", "opencode", "kilo", AGENT_COPILOT, "pi"), - "codex": ("codex", AGENT_COPILOT, "pi"), + "claude": ("claude", "opencode", "kilo", "copilot", "pi"), + "codex": ("codex", "copilot", "pi"), "gemini": ("gemini", "opencode", "kilo", "pi", "antigravity"), "oss": ("opencode", "kilo"), } @@ -676,7 +676,7 @@ def configure_shared_state( or "claude" in tools or "opencode" in tools or "kilo" in tools - or AGENT_COPILOT in tools + or "copilot" in tools or "pi" in tools ) want_gemini = ( @@ -689,16 +689,12 @@ def configure_shared_state( ) # Claude's web-search server also needs a Responses-capable model. want_codex = ( - fetch_all - or "codex" in tools - or "claude" in tools - or AGENT_COPILOT in tools - or "pi" in tools + fetch_all or "codex" in tools or "claude" in tools or "copilot" in tools or "pi" in tools ) # Codex smart routing can select OSS models such as GLM, so a Codex-only # configure must persist that discovered family too. want_oss = fetch_all or "opencode" in tools or "kilo" in tools or "codex" in tools - want_copilot = fetch_all or AGENT_COPILOT in tools + want_copilot = fetch_all or "copilot" in tools claude_reason: str | None = None gemini_reason: str | None = None @@ -796,7 +792,7 @@ def configure_shared_state( "gemini": gemini_reason, "codex": codex_reason, "oss": oss_reason, - AGENT_COPILOT: copilot_reason, + "copilot": copilot_reason, } return state @@ -1001,7 +997,7 @@ def _maybe_select_provider_service(tool: str, state: dict) -> dict: back to Databricks when no matching provider services are found or the listing fails and no custom location is offered. """ - if tool not in ("claude", "codex", "gemini", "antigravity", AGENT_COPILOT): + if tool not in ("claude", "codex", "gemini", "antigravity", "copilot"): return state display = TOOL_SPECS[tool]["display"] @@ -1358,7 +1354,7 @@ def _model_values(value: object) -> list[str]: def _status_models(tool: str, state: dict) -> list[str]: """Return the effective model allow-list for one configured agent.""" static_models = _model_values(state.get(f"{tool}_static_models")) - if tool == AGENT_COPILOT: + if tool == "copilot": models = [model.id for model in copilot_agent.copilot_catalog(state)] elif static_models: models = static_models @@ -1381,7 +1377,7 @@ def _status_models(tool: str, state: dict) -> list[str]: def _status_default_model(tool: str, state: dict, models: list[str]) -> str | None: - if tool == AGENT_COPILOT: + if tool == "copilot": return copilot_agent.default_model(state) explicit = state.get(f"{tool}_default_model") if isinstance(explicit, str) and explicit: @@ -1437,7 +1433,7 @@ def _live_status_model_state(state: dict, tools: set[str]) -> tuple[dict, str]: if oss_models: opencode_models["oss"] = oss_models live["opencode_models"] = opencode_models - if AGENT_COPILOT in tools: + if "copilot" in tools: copilot_models = copilot_agent.catalog_from_listings( cached_model_service_entries(workspace), [*claude_models.values(), *codex_models], @@ -3303,7 +3299,7 @@ def _child_owns_stdout(tool: str, tool_args: list[str]) -> bool: tool_args = tool_args[: tool_args.index("--")] if tool == "claude": return any(arg in {"-p", "--print"} for arg in tool_args) - if tool == AGENT_COPILOT: + if tool == "copilot": return any(arg in {"-p", "--prompt"} or arg.startswith("--prompt=") for arg in tool_args) return tool == "codex" and any(arg in {"exec", "e", "app-server"} for arg in tool_args) @@ -3383,7 +3379,7 @@ def _skip_model_discovery( (``family_defaults``). In the last case it is also harmful: discovered family defaults would overwrite that writer's on every launch, and each rewrite of the managed file costs a sudo prompt. """ - if tool == AGENT_COPILOT: + if tool == "copilot": # Copilot registers its discovered catalog, so a managed default alone still needs it. return bool( provider or managed_parent_schema or (managed and managed_static_models(managed, tool)) @@ -3908,7 +3904,7 @@ def _launch_tool( provider=provider, # Copilot picks its start model from its own catalog; the admin's or budget pick leads. # session_model is set for claude only. - launch_model=managed_model if tool == AGENT_COPILOT else session_model, + launch_model=managed_model if tool == "copilot" else session_model, ) print_success(f"Starting {TOOL_SPECS[tool]['display']}") with _smart_routing_v2_flag(True if managed_smart_routing_applies else None, agent=tool): @@ -4382,7 +4378,7 @@ def copilot_cmd( ) -> None: """Launch GitHub Copilot CLI via Databricks.""" _launch_tool( - AGENT_COPILOT, + "copilot", ctx, provider=provider, skip_preflight=skip_preflight, @@ -4681,8 +4677,8 @@ def configure( requested_agents = [a.strip().lower() for a in (agents or "").split(",") if a.strip()] # Both flags choose VS Code's models alone, so another model agent in the same run # would silently keep the workspace's own. Cursor has no models to choose. - if AGENT_VSCODE not in requested_agents or [ - a for a in requested_agents if a not in (AGENT_VSCODE, "cursor") + if "vscode" not in requested_agents or [ + a for a in requested_agents if a not in ("vscode", "cursor") ]: raise RuntimeError( "--provider and --model-location work only with `--agents vscode`. " @@ -4750,8 +4746,8 @@ def configure( # VS Code is configure-only too: it needs the workspace, then its own setup below. requested = [a.strip().lower() for a in agents.split(",") if a.strip()] wants_cursor = "cursor" in requested - wants_vscode = AGENT_VSCODE in requested - model_agent_names = ",".join(a for a in requested if a not in ("cursor", AGENT_VSCODE)) + wants_vscode = "vscode" in requested + model_agent_names = ",".join(a for a in requested if a not in ("cursor", "vscode")) if model_agent_names: selected_tools = _parse_agents_option(model_agent_names) if workspace_entries is None: diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 3166e8ee3..ad66a2059 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -18,7 +18,7 @@ from ucode.agents import antigravity, claude, codex, copilot, cursor, gemini, kilo, opencode from ucode.config_io import read_json_safe, read_toml_safe -from ucode.constants import AGENT_COPILOT, AGENT_VSCODE, MCP_CLEANUP_SCOPES, MCP_USER_SCOPE +from ucode.constants import MCP_CLEANUP_SCOPES, MCP_USER_SCOPE from ucode.databricks import ( AIGW_MCP_SERVICES_SEGMENT, AuthTokenError, @@ -108,7 +108,7 @@ "display": "Kilo", "list_command": "kilo mcp list", }, - AGENT_COPILOT: { + "copilot": { "binary": "copilot", "display": "GitHub Copilot CLI", "list_command": "copilot mcp list --json", @@ -265,17 +265,17 @@ def _with_vscode_copilot(state: dict, clients: list[str]) -> list[str]: """``clients`` plus ``copilot`` when VS Code is set up for this workspace.""" if not state.get(VSCODE_CONFIGURED_KEY): return clients - return [client for client in MCP_CLIENTS if client in clients or client == AGENT_COPILOT] + return [client for client in MCP_CLIENTS if client in clients or client == "copilot"] def mcp_clients_for_agents(agents: set[str]) -> set[str]: """``--agents`` names as MCP clients.""" - return {AGENT_COPILOT if agent == AGENT_VSCODE else agent for agent in agents} + return {"copilot" if agent == "vscode" else agent for agent in agents} def mcp_client_display(client: str, state: dict) -> str: if ( - client == AGENT_COPILOT + client == "copilot" and state.get(VSCODE_CONFIGURED_KEY) and not shutil.which(str(MCP_CLIENTS[client]["binary"])) ): @@ -289,7 +289,7 @@ def configured_mcp_clients(state: dict, installed_clients: list[str]) -> list[st configured_tools = [] configured = set(configured_tools) if state.get(VSCODE_CONFIGURED_KEY): - configured.add(AGENT_COPILOT) + configured.add("copilot") installed_clients = _with_vscode_copilot(state, installed_clients) return [ client @@ -444,7 +444,7 @@ def configure_client_mcp_server( if client == "kilo": removed = kilo.write_mcp_server_config(name, argv) return [MCP_USER_SCOPE] if removed else [] - if client == AGENT_COPILOT: + if client == "copilot": removed = copilot.write_mcp_server_config(name, argv) return [MCP_USER_SCOPE] if removed else [] if client == "cursor": @@ -469,7 +469,7 @@ def remove_client_mcp_server(client: str, name: str) -> list[str]: return [MCP_USER_SCOPE] if opencode.remove_mcp_server_config(name) else [] if client == "kilo": return [MCP_USER_SCOPE] if kilo.remove_mcp_server_config(name) else [] - if client == AGENT_COPILOT: + if client == "copilot": return [MCP_USER_SCOPE] if copilot.remove_mcp_server_config(name) else [] if client == "cursor": return [MCP_USER_SCOPE] if cursor.remove_mcp_server_config(name) else [] @@ -488,7 +488,7 @@ def revert_mcp_configs(state: dict) -> dict[str, bool]: # One agent's unreadable config must not stop the revert of the others. try: if copilot.remove_legacy_mcp_config(): - results[AGENT_COPILOT] = True + results["copilot"] = True except RuntimeError as exc: print_warning(f"Could not remove ug's old Copilot MCP file: {exc}") for server in all_servers: @@ -1476,7 +1476,7 @@ def _discover_selected_mcp_sources( "claude": claude, "codex": codex, "gemini": gemini, - AGENT_COPILOT: copilot, + "copilot": copilot, "cursor": cursor, "opencode": opencode, "kilo": kilo, @@ -1520,7 +1520,7 @@ def _managed_mcp_entry( return cursor.build_mcp_server_entry(argv) if client == "gemini": return gemini.build_mcp_server_entry(argv) - if client == AGENT_COPILOT: + if client == "copilot": return copilot.build_mcp_server_entry(argv) if client == "opencode": return opencode.build_mcp_server_entry(argv) @@ -2574,7 +2574,7 @@ def parse_mcp_list_output(client: str, output: str) -> dict[str, str]: if client in ("codex", "antigravity"): # Both print a columnar table with an enabled/disabled state column. parsed = _parse_codex_mcp_list(output) - elif client == AGENT_COPILOT: + elif client == "copilot": parsed = _parse_copilot_mcp_list(output) elif client in ("kilo", "opencode"): parsed = _parse_tree_mcp_list(output) @@ -2620,7 +2620,7 @@ def _run_mcp_list(client: str) -> str | None: def query_live_mcp_status(client: str) -> dict[str, str]: """Live ``{server_name: state}`` for one installed agent (empty if its listing can't be read).""" - if client == AGENT_COPILOT and not shutil.which(str(MCP_CLIENTS[client]["binary"])): + if client == "copilot" and not shutil.which(str(MCP_CLIENTS[client]["binary"])): # VS Code only: no Copilot CLI to ask, so report what Copilot's user config holds. return dict.fromkeys(copilot.user_mcp_server_names(), LIVE_ENABLED) output = _run_mcp_list(client) @@ -2853,7 +2853,7 @@ def list_mcp_command(agents: set[str] | None = None) -> int: # Only mention the enabled/disabled caveat for the agents actually in the reported set. config_state_clients = [ mcp_client_display(client, state) - for client in ("codex", AGENT_COPILOT) + for client in ("codex", "copilot") if client in probe_clients ] if config_state_clients: From 22a31b1ca1b5ec8fe9068c6ae0cbf7a1f97ac828 Mon Sep 17 00:00:00 2001 From: Xi Lyu Date: Fri, 30 Oct 2026 15:30:56 +0000 Subject: [PATCH 11/11] copilot: keep a copy of a commented MCP config before the Copilot CLI rewrites it When ug can't parse Copilot's user config (comments, trailing commas) it edits it through `copilot mcp add/remove`. The real CLI saves the file as plain JSON, so the comments are lost and it adds "tools": ["*"] to the developer's own entries. Before the first CLI-driven edit ug now keeps the original at ~/.ucode/copilot-mcp-config.original.json, once, and says so. The unit test asserted the file kept its comments, which only held because the CLI was stubbed; it now checks the copy and the note. Co-authored-by: Isaac --- src/ucode/agents/copilot.py | 12 ++++++++++-- tests/conftest.py | 3 +++ tests/test_agent_copilot.py | 34 ++++++++++++++++++++++++++++++++-- 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/ucode/agents/copilot.py b/src/ucode/agents/copilot.py index 4d34efe50..f2737bdca 100644 --- a/src/ucode/agents/copilot.py +++ b/src/ucode/agents/copilot.py @@ -83,7 +83,7 @@ from ucode.smart_routing.routing import unwrap_anthropic_gateway_model from ucode.state import is_tool_managed, mark_tool_managed, save_state from ucode.telemetry import agent_version, ucode_user_agent, ug_version -from ucode.ui import print_warning +from ucode.ui import print_note, print_warning from .args import LaunchOptions, explicit_model_arg_value from .request_tags import request_tags_header_value @@ -95,6 +95,7 @@ # Older ug versions kept their servers in a side file loaded with `--additional-mcp-config`. LEGACY_MCP_CONFIG_PATH = COPILOT_CONFIG_DIR / "ucode-mcp-config.json" LEGACY_MCP_BACKUP_PATH = APP_DIR / "copilot-ucode-mcp-config.backup.json" +MCP_CONFIG_ORIGINAL_PATH = APP_DIR / "copilot-mcp-config.original.json" COPILOT_PROXY_LOG_PATH = APP_DIR / "logs" / "copilot-proxy.log" _REGISTRY_PREFIX = "ug-providers-" _REGISTRY_LOCK_NAME = "ug-providers.lock" @@ -897,7 +898,14 @@ def _apply_mcp_diff(add: dict[str, dict], remove: set[str]) -> set[str]: "agent also skips the file while it has them." ) # Defer to the copilot CLI, which reads the file the way Copilot does, for exactly the - # changed entries. + # changed entries. It saves the file as plain JSON, dropping its comments, so the + # original is kept once for the developer to recover them. + first_cli_edit = (upserts or remove) and not MCP_CONFIG_ORIGINAL_PATH.exists() + if first_cli_edit and backup_existing_file(path, MCP_CONFIG_ORIGINAL_PATH): + print_note( + f"The Copilot CLI saves {path} without its comments; the original is at " + f"{MCP_CONFIG_ORIGINAL_PATH}." + ) removed = {name for name in remove if _remove_mcp_server_via_cli(name) or name in legacy} for name, entry in upserts.items(): _add_mcp_server_via_cli(name, entry) diff --git a/tests/conftest.py b/tests/conftest.py index a7017fab2..dca79c3be 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -126,6 +126,9 @@ def install_test_policy(path, desired_text, *, interactive=False, allow_sudo=Tru monkeypatch.setattr( copilot_mod, "LEGACY_MCP_BACKUP_PATH", state_dir / "copilot-ucode-mcp-config.backup.json" ) + monkeypatch.setattr( + copilot_mod, "MCP_CONFIG_ORIGINAL_PATH", state_dir / "copilot-mcp-config.original.json" + ) monkeypatch.delenv("COPILOT_HOME", raising=False) def reject_privileged_write(path, _desired_text): diff --git a/tests/test_agent_copilot.py b/tests/test_agent_copilot.py index 0315d088d..df94ceba2 100644 --- a/tests/test_agent_copilot.py +++ b/tests/test_agent_copilot.py @@ -928,7 +928,7 @@ def run(argv, **kwargs): monkeypatch.setattr(copilot.shutil, "which", lambda binary: f"/usr/bin/{binary}") return calls - def test_defers_to_the_copilot_cli_for_a_config_it_cannot_parse(self, monkeypatch): + def test_defers_to_the_copilot_cli_for_a_config_it_cannot_parse(self, monkeypatch, capsys): calls = self._stub_cli(monkeypatch) path = copilot.copilot_mcp_config_path() path.parent.mkdir(parents=True, exist_ok=True) @@ -946,9 +946,39 @@ def test_defers_to_the_copilot_cli_for_a_config_it_cannot_parse(self, monkeypatc ["copilot", "mcp", "remove", "svc"], ["copilot", "mcp", "add", "svc", "--", "ug", "mcp-proxy"], ] - assert path.read_text(encoding="utf-8") == jsonc + # The real CLI saves the file as plain JSON, so the original is kept for its comments. + assert copilot.MCP_CONFIG_ORIGINAL_PATH.read_text(encoding="utf-8") == jsonc + assert "without its comments" in " ".join(capsys.readouterr().out.split()) assert not copilot.LEGACY_MCP_CONFIG_PATH.exists() + def test_cli_edits_keep_only_the_first_original(self, monkeypatch, capsys): + self._stub_cli(monkeypatch) + path = copilot.copilot_mcp_config_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("{\n // first\n}\n", encoding="utf-8") + copilot.write_user_mcp_servers({"svc": self.ENTRY}, set()) + capsys.readouterr() + path.write_text("{\n // second\n}\n", encoding="utf-8") + + copilot.write_user_mcp_servers({}, {"svc"}) + + assert copilot.MCP_CONFIG_ORIGINAL_PATH.read_text(encoding="utf-8") == "{\n // first\n}\n" + assert "without its comments" not in capsys.readouterr().out + + def test_a_cli_pass_with_nothing_to_change_keeps_no_original(self, monkeypatch): + calls = self._stub_cli(monkeypatch) + path = copilot.copilot_mcp_config_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("{\n // mine\n}\n", encoding="utf-8") + copilot.LEGACY_MCP_BACKUP_PATH.parent.mkdir(parents=True, exist_ok=True) + copilot.LEGACY_MCP_BACKUP_PATH.write_text("{}", encoding="utf-8") + + copilot.migrate_legacy_mcp_config() + + assert calls == [] + assert not copilot.MCP_CONFIG_ORIGINAL_PATH.exists() + assert not copilot.LEGACY_MCP_BACKUP_PATH.exists() + def test_commented_config_without_the_copilot_cli_is_left_alone(self, monkeypatch): # VS Code alone has no Copilot CLI to edit a file ug can't parse. calls = self._stub_cli(monkeypatch)