Repository navigation
Conversation
Polly AI Review couldn't run: model busyThis check failed because the review model was at capacity. It is a transient gateway condition, not a problem with your PR. Add the review label again or comment Automated by Polly · workflow run |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| legacy = _take_legacy_mcp_servers() | ||
| removed = {name for name in remove if name in servers or name in legacy} |
There was a problem hiding this comment.
what happens if write_json_file fails after this? the side file and its backup are already gone, so ug's servers are lost. write_json_file also isn't atomic. can we unlink the legacy files only after the write succeeds?
Suggestion:
| legacy = _take_legacy_mcp_servers() | |
| removed = {name for name in remove if name in servers or name in legacy} | |
| legacy = _read_legacy_mcp_servers() # read only, no unlink | |
| ... | |
| write_json_file(path, config) | |
| remove_legacy_mcp_config() | |
| return removed |
There was a problem hiding this comment.
Valid, fixed in 47ce769. The side file is now read without deleting it, and remove_legacy_mcp_config() runs only after write_json_file (or the copilot CLI fallback) succeeds, so a failed write leaves it in place. An empty side file is still removed on the no-op path. Covered by test_failed_write_keeps_the_legacy_side_file and test_failed_cli_add_keeps_the_legacy_side_file. Atomic writes are left as they are for parity: Claude's ~/.claude.json and Codex's config.toml writers aren't atomic either.
| try: | ||
| migrate_legacy_mcp_config() | ||
| except RuntimeError as exc: | ||
| print_warning(f"ug's MCP servers aren't available in this session: {exc}") |
There was a problem hiding this comment.
unlink in remove_legacy_mcp_config raises a raw OSError (e.g. read-only ~/.copilot), which this handler doesn't catch, so the whole launch aborts. can we also catch OSError here, or wrap the unlinks?
There was a problem hiding this comment.
Valid, fixed in 1f0a447. remove_legacy_mcp_config now turns an OSError from either unlink into a RuntimeError, so this handler warns and the launch goes on. Covered by test_a_legacy_side_file_that_cannot_be_deleted_raises_a_runtime_error.
| if apply_managed and tool in MODEL_LOCATION_TOOLS: | ||
| _reject_disabled_agent(managed, tool) |
There was a problem hiding this comment.
copilot now hard-fails on _reject_disabled_agent and takes the managed UC parent from _read_policy_for_other_agent. That reads the cached policy in global scope (force_refresh=False). If an admin just enabled copilot or changed its location, can a stale cache reject it or apply the old one? can we read fresh here like claude/codex?
There was a problem hiding this comment.
This isn't changed by this PR (its diff doesn't touch cli.py); the cached read comes from main: _read_policy_for_other_agent keeps the cached policy in global scope by design ("global keeps the cache"). Only Claude and Codex install the machine-wide policy, and launches enforce the installed one, so a fresh read here would let configure use a policy the launch doesn't. #1256 only puts Copilot on that existing path, the same one every non-installing agent uses. If an admin just enabled Copilot, a Claude/Codex ug configure (or the TTL refresh) installs the new policy. Leaving it as is.
| if copilot.remove_legacy_mcp_config(): | ||
| results["copilot"] = True |
There was a problem hiding this comment.
what happens if ~/.copilot/mcp-config.json is malformed? remove_mcp_server_config now raises RuntimeError, which ends ug revert before the Claude/Codex reverts run. Can we catch it and warn here?
There was a problem hiding this comment.
Valid, fixed in 1f0a447. A config Copilot accepts but ug can't parse (JSON with comments) already goes through copilot mcp remove since 47ce769, so it no longer raises. For a file the copilot CLI can't read either, revert_mcp_configs now catches the RuntimeError per (agent, server), warns which server it couldn't remove, and carries on with the other agents and the rest of the revert. This applies to every agent, not just Copilot. Covered by test_one_agents_failed_removal_does_not_stop_the_others.
| 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") |
There was a problem hiding this comment.
_run_mcp_list adds stderr after stdout, so any stderr text (warnings, update notices) leads to "Extra data", returns {}, and every server shows as not registered. Can we use json.JSONDecoder().raw_decode here instead?
Suggestion:
| servers = json.loads(output[output.find("{") :]).get("mcpServers") | |
| start = output.find("{") | |
| if start < 0: | |
| return {} | |
| servers = json.JSONDecoder().raw_decode(output[start:])[0].get("mcpServers") |
There was a problem hiding this comment.
Valid, fixed in 47ce769. _parse_copilot_mcp_list now reads only the leading JSON object with json.JSONDecoder().raw_decode, so stderr after it is ignored. Covered by test_copilot_listing_followed_by_stderr_still_parses.
1f0a447 to
6c1734c
Compare
ug copilot --model-location <catalog>.<schema> registers the schema's model services in the session registry, from the gateway's Anthropic model listing as for Claude Code. Services are sent by full name; the gateway routes them as model services. Services named like Claude or GPT models use their usual wire (GPT 5+ on Responses, since GPT 5+ rejects the temperature Copilot sends on the Anthropic wire); the rest use the Anthropic wire, which the gateway translates for other models. The saved Custom Model Location and the managed unity_catalog_location apply to Copilot like Claude Code and Codex. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Under a managed unity_catalog_location or model_provider_service, Copilot dropped the budget recommendation and the managed default_model. README now lists Copilot for the saved Custom Model Location. Co-authored-by: Isaac <no-reply@databricks.com>
…Code and Codex Unit tests for the saved location and its precedence with --provider, a managed location or provider, the configure picker, and workspace and single-agent configure. Single-agent `ug configure --agent copilot` now applies the managed model source like the other location agents. The launch branch per agent uses the AGENT_* constants. Co-authored-by: Isaac <no-reply@databricks.com>
… for ug agents add The location catalog strips gateway aliases with smart routing's unwrap_anthropic_gateway_model instead of its own pattern. The single-agent AI-tools install forces a managed-config refresh unless one already ran, so ug agents add copilot keeps it. Co-authored-by: Isaac <no-reply@databricks.com>
…gents Co-authored-by: Isaac <no-reply@databricks.com>
… path Since the workspace policy install landed, only Claude Code and Codex install the policy at configure; Copilot reads it, so its AI-tools install still forces a fresh managed-config read. Co-authored-by: Isaac <no-reply@databricks.com>
location_catalog now goes through build_catalog with an Anthropic fallback wire for services whose names don't decide one, so classification, de-duplication and ordering live in one place. Also explain the Copilot location clause on the launch's model-resolution branch. Co-authored-by: Isaac <no-reply@databricks.com>
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 <no-reply@databricks.com>
… 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 <no-reply@databricks.com>
- `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 <no-reply@databricks.com>
6c1734c to
1b6ec47
Compare
🥞 Stacked PR
Use this link to review incremental changes.
What did you change, and why?
Change: 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.jsonand Codex's~/.codex/config.toml. The ug-owned side file~/.copilot/ucode-mcp-config.json, which onlyug copilotloaded through--additional-mcp-config, goes away.Copilot already configured, discovered and called ug's MCP tools and skills through
ug copilot. The side file caused the gaps compared with Claude Code and Codex:ug mcp listmissing:copilot mcp listnever reads the side filecopilot mcp list --json(enabled/disabled, as for Codex)copilotclaude/codexug revertCOPILOT_HOMECLAUDE_CONFIG_DIR/CODEX_HOMEug copilotlaunch. The side file and its backup are deleted only after that write succeeds.ug revertdeletes the side file.copilot mcp remove/copilot mcp add, as it does for Claude Code and Codex, instead of rewriting the file. A file the CLI can't read either is left untouched and the command fails.ug copilotpasses--allow-all-mcp-server-instructions. ug runs Copilot in offline mode, which empties Copilot's allowlist of servers whoseinstructionsreach the model, so the gateway's servers (Genie, web search, GitHub) lost theirs. Claude Code uses server instructions.Nothing else changes: managed MCP servers and skills already reached Copilot when a managed config enables it, and skills already land in
~/.agents/skills, which Copilot reads.How do you know it works?
Testing:
$COPILOT_HOME; a commented config is edited through the copilot CLI and left as is; a failed CLI add or file write keeps the side file; side-file servers move into the user config and the side file is deleted; revert removes ug's servers and the side file;copilot mcp list --jsonparsing, including stderr after the JSON; theug mcp listnote; launch arguments.just lintis clean.test_ug_copilot_mcp.py: over a developer's own disabled server,ug mcp add --agents copilot --names system.ai.web_search, then Copilot'scopilot mcp list --jsonandug mcp listshow it enabled, a headlessug copilottask calls the tool (checked in Copilot's session events, loaded from the user config), and revert leaves only the developer's server;test_ug_configure_managed_mcp.pyandtest_ug_configure_managed_skills.py(newcopilot_mcp/copilot_skills_by_namefixtures), run by a new non-blockingmanaged-copilotjob;tests/AGENTS.mdnow admits Copilot MCP/skills journeys matching the Claude/Codex ones.main.default.forkable-mealslives on the managed e2e workspace). A commented user config plus an old side file, through the real copilot CLI: the side-file entry and a new server were added, the developer's server kept. By hand, with ug from this branch:ug mcp add --agents copilotwrote the user config, kept a developer's disabled server, moved an old side-file entry over and deleted the side file;ug mcp listshowedenabled;ug copilot -pcalled the web search tool successfully; revert removed ug's server and kept the developer's. The new personal journey couldn't run locally (that workspace's managed config doesn't enable Copilot); this PR's Copilot lane runs it.Limits:
ug mcp-proxy; native HTTP OAuth isn't in this PR.COPILOT_HOMEset to another directory, ug's MCP servers follow it, but downloaded and managed skills still go to~/.agents/skills, which Copilot then doesn't read. Claude Code has the same gap withCLAUDE_CONFIG_DIR.COPILOT_HOMEbetweenug mcp addandug revertleaves the servers in the old location, as forCLAUDE_CONFIG_DIRandCODEX_HOME.Stacked on #1256.
This pull request and its description were written by Isaac.