Repository navigation
copilot: discover and switch between all authorized models - #1218
Conversation
70b8f0e to
5b488aa
Compare
Polly AI ReviewBlocking issuesNone confirmed from the frozen diff and available context. Security vulnerabilitiesNo new vulnerability confirmed by static inspection. Inherited Databricks credentials predate this change; removing them without preserving MCP authentication could break existing launches. Non-blocking notes
ApproachThe approach is sound: reuse the loopback proxy, shared token selector, cross-platform locks, and existing evidence adapters. Keep model and argument matrices local; reserve live journeys for native provider behavior, authentication, migration, and same-session TUI switching. SummaryDiff size (computed): 40 files; +2811 / -544 lines (3355 changed text lines); 0 binary files.
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements. ChangesThe intended outcome is discovering compatible authorized Claude/GPT models and switching between them within one Copilot session. The diff adds classification, managed catalogs, launch selection, private session registries, proxy authentication, cleanup, diagnostics, documentation, and supporting CI coverage. Most implementation breadth follows that stateful workflow. Shared authentication extraction also explains the Codex changes. Concrete reductions are available in repeated headless journeys and duplicate setup, rather than removing distinct provider or lifecycle boundaries. The frozen snapshot’s checksum matches the supplied value. Both trusted review policies were read in full. Review was static: no PR code, tests, lint, or live gateway calls were executed. The declared head was unavailable locally, so findings use the frozen diff’s new-file coordinates. TestsTest-by-test assessmentRows group named tests only where the assessment and recommendation match. Parameterized rows include all their parameters. Removed cases are identified explicitly.
Existing cache coverage in The concrete coverage gaps are direct tests of the new Copilot evidence parser and actual clean-stdout routing. The managed doctor discrepancy needs a focused managed-overlay regression case when corrected. None of these missing tests alone establishes an additional correctness bug. For validation, use a disposable head checkout without credentials first: In a separately authorized live environment, configure once, complete a Claude task, switch to GPT with ScopeNo clearly unrelated change was established. Discovery, proxy authentication, registry lifecycle, managed selection, diagnostics, migration, documentation and CI support the stated Copilot workflow. Codex test adjustments follow the shared authentication extraction. The truncated description limits conclusions about omitted context; it does not justify treating inherited stack changes as unrelated. Automated review by Polly · workflow run |
5b488aa to
253b9c8
Compare
060eea1 to
89e47e1
Compare
…atabricks#1217) ## 🥞 Stacked PR Use this [link](https://github.com/databricks/unity-gateway/pull/1217/files) to review incremental changes. - [**stack/copilot-auth-proxy**](databricks#1217) [[Files changed](https://github.com/databricks/unity-gateway/pull/1217/files)] ← _this PR_ - [stack/copilot-model-discovery](databricks#1218) [[Files changed](https://github.com/databricks/unity-gateway/pull/1218/files/629a83492996f5e7e641695f2f9d54b01d77bfbb..060eea1216d8ab1235712ec0748b427a8ed699c6)] - [stack/copilot-provider-services](databricks#1220) [[Files changed](https://github.com/databricks/unity-gateway/pull/1220/files/060eea1216d8ab1235712ec0748b427a8ed699c6..44f4efa48fd129d5f774649f927bceb5d751c0c2)] --------- ## What did you change, and why? **Change:** `ug copilot` now runs Copilot CLI against a per-session loopback proxy (`gateway_proxy.start_gateway_auth_proxy`) instead of handing it a Databricks token. - Copilot gets `COPILOT_PROVIDER_BASE_URL=http://127.0.0.1:<port>/ai-gateway/...` and a random per-session secret as `COPILOT_PROVIDER_BEARER_TOKEN`. - The proxy accepts only that secret (Bearer or `x-api-key`) and only on `/ai-gateway/{anthropic,mlflow/v1,openai/v1}/` (decoded until stable; `.`/`..` segments rejected). - It swaps the secret for a fresh Databricks token from the CLI profile or custom OAuth, using `TokenCache` (refresh near expiry, force refresh plus one retry on 401/403), and adds `ucode/<v> copilot/<v>` User-Agent, request tags, and managed `copilot_http_headers`. - If refresh fails, Copilot gets a dialect-shaped 401 ("Databricks sign-in expired. Run `databricks auth login --host <ws>`, then restart `ug copilot`."). The error goes to `~/.ucode/logs/copilot-proxy.log`, not over the TUI. - Configure and launch persist no credential. Tokens written by earlier versions are scrubbed from `~/.copilot/ucode.env` and from a UG-produced backup, including before `ug revert`, so revert cannot restore one. - Removes the Copilot token-refresher thread and its SIGINT forwarding; the parent keeps waiting on Ctrl-C so the proxy outlives Copilot. - Drops inherited `COPILOT_PROVIDER_API_KEY` / `_API_KEY_COMMAND` / `_TRANSPORT` so they can't bypass the proxy. - Adds a non-blocking `copilot` integration lane (Linux + Windows, `live and copilot`, outside `cujs`) mirroring OpenCode's, and amends the `tests/AGENTS.md` scope line to allow Copilot headless tests. **Why:** Copilot reads its provider credential once at startup. - **Expiry:** the old refresher rewrote `ucode.env`, which Copilot never re-reads, so sessions failed once the OAuth token expired (~1h). - **Plaintext tokens:** the token sat in plaintext in `ucode.env`, its backup, and the child env. - **Shared-state writes:** the refresher also called `save_state` from a background thread, which could reset the active workspace during a concurrent `ug configure`. BEFORE: a `ug copilot` session stops working after token expiry until restarted; `~/.copilot/ucode.env` and `~/.ucode/copilot-ucode-env.backup` contain the Databricks token; Ctrl-C on Windows raises `ValueError` from `send_signal(SIGINT)`. AFTER: requests carry a freshly minted token for the life of the session; no file or Copilot env var holds a Databricks credential; Ctrl-C is delivered to Copilot by the terminal and UG tears the proxy down after Copilot exits. ## How do you know it works? **Testing:** - `tests/test_gateway_proxy_integration.py::TestGatewayAuthProxyEndToEnd` (real sockets): - secret swap and added headers; - `x-api-key` accepted and not forwarded; - a wrong or missing secret gets a 401 and nothing is forwarded; - path refusals (`../`, `%2e%2e`, `%252e%252e`, otel); - a 401 from the gateway triggers a forced refresh and one retry; - the actionable 401 is shaped for each dialect, with nothing on stderr. - `tests/test_agent_copilot.py`: - no credential in the env overlay or on disk; - scrubbing legacy tokens and UG-produced backups; - token provider for both the CLI profile and custom OAuth; - header precedence; - launch through the real proxy with a stubbed child process. - `tests/test_e2e.py::TestCopilotLaunch` now runs through the real proxy (and patches `ucode.agents.copilot.save_state`, which the old patch target missed). - New `tests/integration/test_ug_copilot_headless.py`: a Claude (Anthropic) task and a GPT (Responses) task, each asserting no `DATABRICKS_BEARER` on disk under `~/.copilot` / `~/.ucode`. **Not run yet**: needs the e2e workspace and Copilot 1.0.91. This draft is so the lane can run in CI. - Locally on Linux: 807 passed across the touched suites; `just lint` (ruff check/format, ty) passes. The remaining unit failures in `TestInstallToolBinary` / `test_e2e_user_agent` also fail on a clean `main` checkout here. **Open questions for reviewers:** - `tests/AGENTS.md` scope change needs CODEOWNERS sign-off. - Revocation returns an actionable 401 rather than retrying with a browser login, because UG's login path writes to the terminal Copilot owns. A browser-login retry is a follow-up. - A user `~/.copilot/providers.json` can still override UG's provider; guard left for a follow-up. - Still to check live: how Copilot renders the proxy's 401 body; Windows firewall behavior on the loopback bind. Not in this PR: binary resolver (databricks#970), version floor / wizard / `forceLoginOrgs` doctor, `--workspace`, `-p` stdout banner, MPS, OTEL, OSS models. This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com> Co-authored-by: Rohit Agrawal <10210921+rohita5l@users.noreply.github.com>
c5f22a4 to
4dcc345
Compare
Polly AI ReviewBlocking issues
Security vulnerabilitiesNo confirmed security vulnerability found. Registry creation, credential separation, environment isolation, and cleanup were inspected statically; Windows permissions were not runtime-verified. Non-blocking notes
ApproachThe per-session registry and existing proxy are a sound fit. Reuse the shared discovery path’s per-family fallback, preserve UC exclusions for matching IDs, and use the existing registry mutex for finalization. SummaryDiff size (computed): 28 files; +1667 / -482 lines (2149 changed text lines); 0 binary files.
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements. ChangesThe change replaces single-provider Copilot configuration with a session registry, adds authorized-model discovery and selection, carries managed defaults into launch, and exposes the catalog through status and doctor. The diff includes implementation, replacement unit coverage, live journeys, evidence helpers, runner support, and documentation. There are no dependency or generated-file changes. Most of the size follows from replacing the previous configuration contract and covering its lifecycle. No separate implementation-only refactor clearly needs removal. TestsTest-by-test assessmentThis is a static assessment, not a passing test report. The snapshot checksum and both trusted policy files were verified. No PR code or tests were executed, and no outbound requests were made. Native Copilot behavior and the Windows cleanup interleaving remain runtime-unverified. The trusted checkout lacks Tooling limitation: the OpenCode, Cursor, Hermes, Pi, and Antigravity CLIs were unavailable. “Removed” identifies deleted coverage. Grouped rows name tests that protect the same contract or share the same disposition.
The demonstrated gaps are partial UC discovery, GPT-4o classification, and concurrent Windows cleanup. Existing proxy socket and unchanged authentication-recovery coverage suffice for token refresh; no additional live authentication journey is requested. After fixes, run focused checks in a credential-isolated environment with dependencies already installed: uv run --offline pytest tests/test_agent_copilot.py tests/test_cli.py tests/test_databricks.py tests/test_doctor.py tests/test_integration_evidence.py tests/test_managed_resolve.py tests/test_managed_setup.pyFor manual verification, use Copilot 1.0.91 or newer in a disposable home. Configure once, complete a Claude file task, switch with ScopeThe inspected changes support discovery, selection, session isolation, configuration compatibility, diagnostics, or verification of those behaviors. No clearly unrelated changes were found. Whether a managed default outside Automated review by Polly · workflow run |
Co-authored-by: Isaac <no-reply@databricks.com>
….json Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
The e2e workspace lists claude-sonnet-4-5 but the CI principal gets 403 on it; claude-haiku-4-5 is already older than its family's newest there. Co-authored-by: Isaac <no-reply@databricks.com>
- A workspace switch no longer carries the source workspace's Copilot catalog when the destination's listing fails. - ug doctor counts the managed model list that launch registers. - Dotted model names keep their version punctuation when classified. - Copilot evidence parsing and -p stdout routing get unit coverage. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
…tion journeys Copilot's shell tool tries pwsh first; with only Windows PowerShell on the test PATH its 40s startup probe can time out, leaving Copilot without a shell, so the revoke/sign-out step never runs. Expose pwsh to Copilot lanes on Windows. gpt-5-4-nano sometimes drops the path segment after the space in 'project with spaces' when it rebuilds the file's absolute path. Give the GPT journeys the full path. Co-authored-by: Isaac <no-reply@databricks.com>
…s test PATH Co-authored-by: Isaac <no-reply@databricks.com>
… and discovery - A managed model_config.models list picks the start model again, as before. - An unservable managed entry or a repeated candidate no longer warns twice. - COPILOT_PROVIDER_WIRE_MODEL still sets the start model's served name. - A failed Copilot session-file sweep warns instead of stopping ug revert. - Copilot skips discovery when a provider, parent schema or managed list names its models. - Drop the unreachable copilot status fallback. Co-authored-by: Isaac <no-reply@databricks.com>
…ns under the mutex - A UC listing without Claude or GPT models no longer discards the models the gateway listings found for that family, so a Gemini-only listing doesn't leave Copilot with an empty catalog. - gpt-4o and gpt-4o-mini classify as Completions models again. - Session cleanup holds the registries mutex, so a concurrent sweep can't reopen the lock file before it is deleted (a PermissionError on Windows). Co-authored-by: Isaac <no-reply@databricks.com>
d27ae5f to
fe24b4f
Compare
…#1220) ## 🥞 Stacked PR Use this [link](https://github.com/databricks/unity-gateway/pull/1220/files) to review incremental changes. - [stack/copilot-auth-proxy](databricks#1217) [[Files changed](https://github.com/databricks/unity-gateway/pull/1217/files)] [MERGED] - [stack/copilot-model-discovery](databricks#1218) [[Files changed](https://github.com/databricks/unity-gateway/pull/1218/files)] [MERGED] - [**stack/copilot-provider-services**](databricks#1220) [[Files changed](https://github.com/databricks/unity-gateway/pull/1220/files)] ← _this PR_ - [stack/copilot-model-location](databricks#1256) [[Files changed](https://github.com/databricks/unity-gateway/pull/1256/files/a6c28cbc82fe5d36f47fdbf8f06744d4a2019427..f8ffea64fa222d47f61b3e53375bca312ebc780a)] --------- ## What did you change, and why? **Change:** `ug copilot --provider <catalog>.<schema>.<name>` runs Copilot through a Unity Catalog Model Provider Service, with that service's models in `/model`. Claude Code and Codex already support provider services; Copilot refused all of them ("supported: none"). - **Routing.** The session registry from databricks#1218 gets the service's models. Every registry provider carries `headers: {"Databricks-Model-Provider-Service": <service>}`, which Copilot sends on every request, and the loopback proxy forwards it unchanged. The proxy removes `reasoning_effort` from Chat Completions requests: Copilot sends it, and OpenAI and Azure reject it for the models on that wire. - Claude models -> `dbx-anthropic` on `/ai-gateway/anthropic`. - Other chat models -> `dbx-responses` (GPT 5+ and o-series, which reject Copilot's `temperature: 0` on Chat Completions) or `dbx-completions`, both on `/ai-gateway/openai/v1`, the gateway's native OpenAI route, which serves provider services untranslated. Databricks-hosted models keep `/ai-gateway/mlflow/v1`. - **Provider types.** - Supported: `anthropic`, `amazon_bedrock` and `bedrock_mantle` (Claude targets only), `openai`, `azure_openai`. - Refused with an actionable error: subscription-relay Anthropic services (Claude Code only), Bedrock services without Claude targets, other provider types. `microsoft_foundry` is left for a follow-up once there is a test fixture for it. - **Models.** - Provider ids are classified whole (`gpt-5.5`, `gpt-4o`, `us.anthropic.claude-sonnet-4-6`). The `system.ai` name logic took the text after the last dot, which dropped `gpt-5.5` and `gpt-4o`. - Dated, versioned and region-less Claude ids (`claude-sonnet-4-5-20250929`, `...-20250929-v1:0`, `anthropic.claude-opus-4-8`) map to Copilot's catalog id (`claude-sonnet-4.5`), so Copilot sends the right Claude settings. Provider Claude models whose catalog limits exceed 200K are capped at 200K, because a provider may not serve the 1M window. - Declared targets are used as is. A service that allows every model registers the gateway's listing for it (Anthropic `/v1/models`, Codex `/v1/models`) plus `--model` (plain id or `dbx-<wire>/<id>`) or the managed default. Without either, the session starts on Copilot's default pick from the listing, as `ug codex` and `ug claude` do; the listing is a fixed catalog, not what the provider account serves, so that pick can fail. An empty or failed listing asks for `--model`, and a failed listing is reported with its cause. - Targets whose declared APIs (`native_api_types`) include no chat API, such as embedding deployments, are skipped with a warning. The declared APIs only exclude; the wire still comes from the model name. GPT and o-series versions sort newest first. - **Selection.** `ug configure` offers matching services for Copilot under External Models, and a managed config's `model_provider_service` for Copilot applies, the same way as for Claude Code and Codex. Under a provider the managed Databricks model list is ignored. - **Workspace.** `ug copilot --workspace <url>` targets that workspace for the launch and sets it up first if needed, as `ug claude` and `ug codex` do. Before, the flag was passed through to Copilot. ## How do you know it works? **Testing:** - **Unit:** - provider classifier (wire per provider type, non-chat and non-Claude exclusions), catalog order, Claude catalog ids for dated/Bedrock ids; - provider catalog: declared targets, allow-all listing for Anthropic/Bedrock and OpenAI, `--model` on allow-all, empty listing, no usable model, skipped-model warning, invalid service; - registry rendering with the header, native OpenAI base URL and 200K Claude limits; - launch through the real proxy with a stubbed Copilot process: registry content, start model, a model the service doesn't serve fails before Copilot starts; - `resolve_provider_service` / `service_usable_for_tool` for Copilot, and `ug copilot --provider` reaching the launch. - The full unit suite passes except tests that also fail on the base in this environment (`TestInstallToolBinary`, `test_e2e_user_agent`, three `test_smart_router` cases). `just lint` is clean. - **Integration (new `tests/integration/test_ug_copilot_providers.py`, non-blocking Copilot lane):** file tasks through the existing Anthropic MPS fixture (no `--model`), the OpenAI and Azure OpenAI MPS fixtures on `gpt-5-nano`, and the Bedrock allow-all fixture with a named model, with the answering model checked from Copilot's own session events; the relayed fixture fails before Copilot starts. These run in CI for the first time on this PR. - **Live (`ug` from this branch, one provider service per row unless noted):** One headless `ug copilot --provider <service> [--model <id>] -p "Reply with exactly: OK"` per model, plus each service once without `--model` to check the start model. The model shown is the one Copilot's session log says answered. | Provider type | Model | Copilot wire | Result | |---|---|---|---| | OpenAI | `gpt-5.4-nano`, `gpt-5.6-luna`, default -> `gpt-5.6-luna` | Responses | ✅ OK | | OpenAI (allow-all) | `gpt-5-nano`, default -> `gpt-6.1-sol` (from the gateway listing) | Responses | ✅ OK | | OpenAI (allow-all) | `gpt-4o-mini` | Chat Completions | ✅ OK | | Azure OpenAI | `gpt-5.5`, `gpt-5.6-luna`, default start model | Responses | ✅ OK | | Azure OpenAI | `gpt-4.1-mini`, `gpt-4o-mini` | Chat Completions | ✅ OK | | Anthropic | `claude-haiku-4-5`, `claude-sonnet-5`, default -> `claude-sonnet-5` | Anthropic | ✅ OK | | Anthropic (allow-all) | `claude-haiku-4-5`, `claude-sonnet-4-5`, default -> `claude-sonnet-5-5` (from the gateway listing) | Anthropic | ✅ OK | | Amazon Bedrock (with non-Claude targets) | `us.anthropic.claude-haiku-4-5-20251001-v1:0`, `us.anthropic.claude-sonnet-4-6`, default -> `us.anthropic.claude-sonnet-4-6` | Anthropic | ✅ OK; the service's non-Claude targets (`grok-4.6`, `gpt-oss`) are left out of `/model`, with a warning | | Amazon Bedrock (service credential, allow-all; 3 services) | `us.anthropic.claude-haiku-4-5-20251001-v1:0` | Anthropic | ✅ OK | | Relayed Anthropic subscription | - | - | ✅ refused before launch: "relays a Claude subscription, which only Claude Code can use" | **Not in this PR:** `--model-location` for Copilot (next), Microsoft Foundry services (needs a fixture), mixing Databricks and provider models in one session. Stacked on databricks#1218. This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com>
🥞 Stacked PR
Use this link to review incremental changes.
What did you change, and why?
Change:
ug copilotnow registers every Claude and GPT model the user can use on the workspace and lets them switch between them inside one Copilot session.COPILOT_PROVIDERS_CONFIG) to~/.copilot/ug-providers-<id>.json(0600), deleted when the session ends. Each session holds a lock on a sibling.lockfile, and registries whose lock nobody holds (a killed session) are swept at the next launch and byug revert. The user's own~/.copilot/providers.jsonis never touched.dbx-anthropic(Anthropic Messages),dbx-responses(Responses, GPT-5+) anddbx-completions(Chat Completions, older GPT). All of them point at the existing loopback auth proxy with the session secret; only providers that have models are written.modelId, so Copilot sends Claude request settings, plus prompt/output limits. Without explicit limits Copilot assumes a 128K prompt for registry models.COPILOT_PROVIDERS_CONFIG,COPILOT_MODEL=<provider>/<id>andCOPILOT_OFFLINE=true, and drops inheritedCOPILOT_PROVIDER*settings. The single-provider env (COPILOT_PROVIDER_TYPE/BASE_URL/WIRE_API/MODEL_ID) and theCOPILOT_PROVIDER_WIRE_MODELoverride are gone.supported_api_types. When that listing is empty, it falls back to the gateway listings. No extra requests.claude-*(including baredatabricks-claude-*, GitHub Copilot does not work with Anthropic models #108) -> Anthropic;gpt-N-> Responses for N >= 5, else Completions. Advertised API types only exclude.stream_options), open-source models (not validated with Copilot yet), non-chat models,claude-opus-4-1(the gateway 400s Copilot's thinking settings).--model/-maccept a short name,system.ai.*or Copilot'sprovider/id. They're rewritten toprovider/id, because Copilot only matches that form; Copilot has no-m, so it becomes--model.--model, then the budget recommendation or managed default (now passed throughLaunchOptions.launch_model; it was ignored for Copilot before), then the managed list, then the newest Sonnet/Opus/Haiku, then GPT.model_serviceslist for Copilot now reaches state through the existing static-models seam and replaces discovery. Entries Copilot can't serve are skipped with a warning, and the wizard's Copilot families drop Gemini/OSS.ug statuslists Copilot's catalog and default, andug doctorreports the model counts.ug copilot -pkeeps stdout for Copilot by moving ug's banners to stderr.launch_tokenhelper is reused.Why: Copilot's env-var BYOK supports one provider per session, so
/modelcouldn't reach Databricks models across families, and switching between Claude and GPT meant relaunching. Copilot also only saw the newest model per Claude family plus the Codex catalog, which leakedgpt-oss. Copilot doesn't discover gateway models itself the way Claude Code and Codex do, so ug builds the catalog.BEFORE:
ug copilotpins one model and wire per launch, and/modelcan't switch to another Databricks model family.databricks-claude-*ids don't reach Copilot or take the OpenAI path.AFTER:
/modellists all authorized Claude and GPT models (on dogfood: 14 Claude + 18 GPT) and switches across providers mid-session, carrying tool history.--modelaccepts friendly names and fails fast with the list.How do you know it works?
Testing:
type/wireApi/baseUrl), file mode and cleanup, sweep of dead sessions, env, version gate;-pstdout, the listing's API types;tests/test_agent_copilot.pyruns the real proxy with a stubbed Copilot process.TestInstallToolBinary/test_e2e_user_agent, which also fail onmainin this environment.just lintis clean.tests/integration/test_ug_copilot_models.py, non-blocking Copilot lane):dbx-responses/<model>;/model dbx-responses/<model>, completes a GPT task and/exits.session-state/*/events.jsonl(assistant.message.model,session.start.selectedModel,session.model_change.newModel). The existing headless journeys now also assert the answering model and that no registry is left.ugfrom this branch, dogfood):claude-haiku-4-5,dbx-responses/system.ai.gpt-5-4-nanoandclaude-sonnet-4-5headless tasks completed with clean stdout;system.ai.gemini-3-flashfailed before launch with the list;/model dbx-responses/system.ai.gpt-5-4-nanoprinted "Model changed ... for this session", and GPT answered;Not in this PR: open-source models (next on this stack), Model Provider Service models in the registry, GPT token limits, mixing GitHub-seat models into
/model(depends on the seat-based-auth decision).Stacked on #1217.
Test infra deflakes (Copilot lanes): on Windows, a dedicated
pwshinstall directory is added to the isolated test PATH, because Copilot's shell tool triespwshfirst and Windows PowerShell alone can miss its 40s startup probe, which left the revoke step with no shell. The Copilot GPT journeys also get the fixture's full path (FileTask.prompt_with_path), because gpt-5-4-nano sometimes dropswith spaceswhen it rebuilds the path.This pull request and its description were written by Isaac.