Repository navigation
feat(vscode): migrate the providers settings to v2 - #15592
Conversation
Add the custom provider dialog, model cards, validation and provider rows (edit and remove) to the v2 Providers tab. Store the API key as a v2 credential and disabled providers as provider.use deny policies. Also guard the alias removal in applyConfigEdits: jsonc-parser throws when a delete targets a path the document does not define.
|
📈 v2 migration progress: 6.3% → 6.4% (+8 done, 0 total, 0 dropped; 626/9819 cells) Placement: 0 new marked lines in shared files, 0 new Ratchets: 27 core-internal imports (base 27); shared patches by rung 35 seam, 25 patch (base 35 seam, 25 patch).
8 cells closed
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The new commit ( Files Reviewed (2 files)
Previous Review Summaries (7 snapshots, latest commit 2072844)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 2072844)Status: 1 Issue Found | Recommendation: Address before merge Overview
The new commit ( Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 355cb2a)Status: 2 Issues Found | Recommendation: Address before merge Overview
The new commit ( Issue Details (click to expand)SUGGESTION
Files Reviewed (7 files)
No memory-leak risk in the changed code: the additions are a validation predicate and a toast, with no listeners, timers or caches added. No markdown documentation files are changed in this PR. Fix these issues in Kilo Cloud Previous review (commit 5dbb526)Status: 2 Issues Found | Recommendation: Address before merge Overview
The new commit addresses the counter-check items: the masked key field now strips stray asterisks on a middle edit, the Delete toast waits for the config removal to succeed, Issue Details (click to expand)SUGGESTION
Files Reviewed (7 files)
No memory-leak risk was found in the new code: the additions are a save-time policy rewrite and a toast guard, and no listeners, timers or caches were added. No markdown documentation files are changed in this PR. Fix these issues in Kilo Cloud Previous review (commit c980a88)Status: No Issues Found | Recommendation: Merge The incremental change extracts the disabled-provider policy logic into Files Reviewed (4 files)
Previous review (commit a5419ad)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
The previous review's findings are addressed in this range: the reasoning merge now preserves No memory-leak risk was found in the new code: no listeners, timers or caches were added, and the new tests are pure helpers. No documentation files are changed in this range. Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 373810c)Status: 5 Issues Found | Recommendation: Address before merge Overview
The merge that preserves unmodeled config fields now survives an edit, the Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Verified while reviewing: Fix these issues in Kilo Cloud Previous review (commit d94d55a)Status: 3 Issues Found | Recommendation: Address before merge Overview
The two problems from the previous review that mattered most are fixed: an edit now merges into the existing Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (12 files)
Verified while reviewing: Reviewed by deepseek-flash-latest · Input: 85.4K · Output: 10K · Cached: 340.7K Review guidance: REVIEW.md from base branch |
…gs-custom-provider
Preserve unmodeled provider and model config fields when the custom provider dialog edits an existing provider, drop the stored env var when a key is typed, handle a rejected credential removal, remove the duplicate model error text, and add tests for provider-visibility and the merge. Merge the foundation config write fix and drop the stray kilo2.json test artifact.
Merge the other input modalities (pdf, audio) instead of replacing the list, clear the stored env only when the API key field is actually changed, and cover both in the tests.
…gs-custom-provider
…gs-custom-provider
The config aliases map disabled_providers, enabled_providers and experimental.policies to policies. Read them so the disabled list shows an existing legacy value and a write converts it instead of dropping it.
Keep the reasoning effort, budget and toggle overlay when the toggle stays on, remove headers when the dialog has none, read the disabled ids from deny policies only, merge against the current config at save time, and drop the legacy provider-list reads the server already normalizes into policies.
An enabled_providers allow-list normalizes to a '*' deny plus allow statements. Skip the catch-all when listing disabled providers and keep it when writing policies, so disabling a provider does not turn the allow-list into default-allow. Covered by tests.
AI counter-check findings
|
WebReflection
left a comment
WebReflection
left a comment
There was a problem hiding this comment.
Counter-review findings in here #15592 (comment)
Approving in case you think those concerns should not be addressed (or have been addressed already).
…gs-custom-provider
Strip the mask characters anywhere, not only at the start, so an edit inside the masked key field cannot persist stray asterisks. Show the disconnect toast only when the config removal succeeded, and re-enable a disabled provider whose id the dialog reuses (the validation allows the reuse, so the save must clear the deny).
|
@WebReflection I am migrating the components as is. I won't fix any bugs that where there previously, could you prompt your review agent to focus on integration issues and migration issues instead? |
What Problem This Solves
The Providers settings tab on v2 could list and connect integration providers, but there was no way to add a custom provider, edit or remove one, or hide a provider. Users who rely on a custom OpenAI-compatible, OpenAI Responses or Anthropic Messages endpoint had no equivalent on v2.
Why This Change Was Made
The tab now owns the custom provider flow on the v2 config and credential APIs:
providers.<id>={ name, package, settings.baseURL, headers, models.<id>.{ name, capabilities } }, written throughuseSettingsConfig().setso the host writes the config file. The dialog maps the model Reasoning toggle tocapabilities.reasoning.supportedand the Image toggle tocapabilities.input(["text"]or["text","image"]). The v1 provider package names map to the built-in v2 packages (@opencode/ai/providers/openai-compatible,.../openai,.../anthropic-compatible), so a saved provider loads without an npm install.settings.connectKey(api.integration.connect.key). Nothing secret is written to the config file; the edit dialog shows the credentials as a mask. The{env:VAR}spelling still writesproviders.<id>.env.policies: [{ action: "provider.use", resource: "<id>", effect: "deny" }], which removes the provider fromprovider.listand from the model list. The section preserves the other policies and unsetspolicieswhen the last deny is removed. A legacydisabled_providersentry arrives as a normalized deny policy, and a legacyenabled_providersallow-list keeps itsresource: "*"catch-all andallowstatements through a write (it is not shown as a disabled provider). The write converts the legacy key intopoliciesbecauseconfig.sourceand the host alias map know it.providers.<id>in the scope that defines it (the host removes theprovider.<id>alias in the same pass) and clears the credential.providers.<id>(read at save time), so fields the dialog does not model survive: other providersettings,body, modellimit,cost,variantsand extra capability keys. Unchecking Image only removesimagefromcapabilities.input, keepingpdf/audio; a reasoning toggle that stays on keeps theeffort,budgetandtoggleoverlay; an untouched API key field keeps the storedenvand credential; and empty headers or env are removed, because the dialog is the source of truth for those fields.This PR also reported a write blocker:
jsonc-parserthrows (Can not delete in empty document) when a delete targets a path the document does not define, which made every write fail unless the legacy alias already existed. The guard landed in the foundation (settings-foundation), so this PR no longer changesconfig-file-edit.ts; the tests here cover the providers case.Parity
providers.<id>throughuseSettingsConfig().set; key throughsettings.connectKeyCustomProviderValidation.validateCustomProvider{env:NAME}key blocks the save with "Request failed: Invalid environment variable name"models.<id>.name,capabilities.reasoning.supported,capabilities.inputproviders.<id>; Edit opens the dialog, Delete unsetsproviders.<id>and clears the credentialcanChangeKeyreturns false for a custom provideropenFileon the defining scope's filepolicieswithaction: "provider.use",effect: "deny"Out of scope, with reasons:
Known divergences (small, deliberate):
policiesinstead of writing[]as v1 did. The resulting policy set is the same, and the file does not keep an empty key.@ai-sdk/anthropicmaps to@opencode/ai/providers/anthropic-compatible, not.../anthropic, because the dialog always supplies a custom base URL (the officialanthropicpackage targets the vendor API).Proof
Screenshots, v1 on the left and v2 on the right.
The v1 dialog header also shows a Fetch models button (out of scope above). The v2 dialog is otherwise the same layout, labels and order.
The model picker shows the custom provider and its model without a server restart:
End-to-end check in an isolated dev profile (VS Code 1.106,
launch-vscode --mode dev, no model prompts):acmeprov, OpenAI-compatible, base URL, one model, API key). The config file receivedproviders.acmeprovwithsettings.baseURLandmodels.acme-chat-1.capabilities.input = ["text"]; no key appears in the file.integration_id=acmeprov,{"type":"key","key":...}.api.model.listand the chat model picker showacmeprov/acme-chat-1at once, with no server restart.policies: [{ action: "provider.use", resource: "<id>", effect: "deny" }]and hid the provider from the list; Enable removed the policy and unset the key when the last deny was removed.providers.acmeprovstill carriedsettings.timeout,body, and the modellimit,cost,variantsandcapabilities.output.{env:lower}keeps the dialog open with the v1 toast "Request failed: Invalid environment variable name"; no config entry and no credential are written.{env:ACME_KEY}savesenv: ["ACME_KEY"]with no credential, and an edit with the key untouched keeps that env.Automated:
bun typecheckandbun run lintpass inpackages/kilo-vscode;bun testpasses 166 tests (0 fail), includingtest/custom-provider.test.ts. The rootbun run checkfails in this environment before reaching this package (bun checkis not a known subcommand for the installed bun, so@opencode/theme,@opencode/utiland@opencode/codemodereportScript not found "check"); CI runs the real check.Progress map
vscode: 1769 to 1766 open cells (3 dispositions). The four ported files close their module cells by name.Done, and why:
webview-ui/src/components/settings/ProvidersTab.tsx,CustomProviderDialog.tsx,CustomProviderModelCard.tsx,CustomProviderValidation.ts,provider-visibility.tsare ported at their v1 paths.src/shared/custom-provider.ts(key spelling and masked key now live in the dialog and validation, the key is a credential and the deep-merge sentinels are gone),tests/unit/custom-provider-dialog-validate.test.tsandtests/unit/provider-visibility.test.ts(assertions carried bypackages/kilo-vscode/test/custom-provider.test.tsand the tab).Still open:
AnacondaDesktopDialog.tsxand the Fetch models behavior are out of scope for this unit, with the reasons above.Part of #14386