fix(jetbrains): resolve api key env var when selecting custom provider models - #13586
Conversation
…r models The custom OpenAI-compatible provider dialog's "Select models" action only read the API key text field, ignoring the "API key environment variable" field entirely. Providers configured with an env var worked for chatting (the CLI resolves it at runtime) but failed to list models unless the key was retyped into the dialog. Resolve credentials backend-side, where the env that matters actually lives (the CLI process), with precedence: typed key -> env var value via EnvironmentUtil -> the provider's already-stored key from provider state. This also removes the retyping requirement for providers authenticated with a stored API key, not just an env var. An unresolvable env var no longer blocks the fetch, since local providers (Ollama, LM Studio, etc.) need no credential; instead the result carries an envMissing flag so the dialog can append a hint naming the variable. While tracing this, three related defects surfaced from the same root cause (env-configured custom providers being second-class in this UI): - saved custom headers were never sent when fetching models - deleting a custom provider whose env var resolved was blocked, because the disconnect guard checked provider source before the config-backed delete branch - clearing the env var field left the old value in kilo.json, since the config patch omitted the key instead of nulling it Fixes all four. The API key field still shows blank on edit (secrets are never prefilled or logged); a hint now explains that a saved key is kept when the field is left blank.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (3 snapshots, latest commit bc07f1e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit bc07f1e)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit a4956d3)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit ad5cf21)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (9 files)
Reviewed by grok-4.6 · Input: 72.4K · Output: 10.4K · Cached: 403.6K Review guidance: REVIEW.md from base branch |
ConfigProviderV1.Info.env is Schema.optional(Array), not NullOr, so a "env": null patch field is rejected by the real CLI's schema-validated PATCH /global/config payload (unlike the whole provider entry, which is Schema.NullOr and does support null for deletion). The previous commit's fix for defect D sent that unsupported field-level null unconditionally, which would 400 every custom-provider save that has no env var configured -- the common case. MockCliServer's merge simulation doesn't enforce the schema, so the added test passed locally without catching this; the automated PR review caught it. Clear the env var by deleting the provider entry first (schema-supported, and a no-op when nothing exists yet) and recreating it right after without an "env" key, instead of trying to null the field directly.
|
Good catch — |
The unconditional delete-then-recreate on every save (added in the previous commit) wiped the whole provider entry on every save that has no env var, which is the common case. That drops any field the recreate patch doesn't set -- e.g. a hand-authored whitelist/blacklist -- and leaves the entry missing if the recreate patch fails partway through. Only take the delete-first path when the provider actually has an existing env var to clear, narrowing both risks to that specific, deliberate action instead of every ordinary save.
|
Addressed in bc07f1e: the delete-then-recreate now only runs when the provider actually has an existing env var to clear (checked via |
Add translations for settings.providers.customEnvMissing and settings.providers.customKeyStored (introduced in this PR) across all 18 existing locale bundles. These files had fallen out of sync with the newer provider-settings section of the base bundle, so this only adds the two keys this PR introduces rather than backfilling the whole section.
Clearing a provider's env var deletes and recreates the whole config entry, because the CLI config schema only accepts null for an entire provider value. The recreate patch is built from the save DTO, and the provider dialog has no headers field, so any headers hand-authored in kilo.json were silently dropped by an unrelated env var change. Carry the existing headers into the recreate patch when the save itself sets none.
Issue
Fixes #
Context
User feedback: a custom OpenAI-compatible provider configured with an "API key environment variable" works for chatting (the CLI resolves the env var at runtime), but Select models silently fails unless the API key is retyped into the dialog every time. The API key field is also blank when reopening the provider editor, which reads as data loss.
Tracing the model-fetch path found the root cause: the JetBrains plugin's "Select models" action never looked at the env var field at all, and had no way to. Digging further surfaced three related defects with the same root cause — env-configured custom providers are second-class in this settings UI.
Implementation
Credential resolution for model fetching now happens backend-side, where the environment that matters actually lives (the CLI process), not in the frontend dialog. In split mode the frontend can run on a different machine than the backend, so resolving in the dialog would resolve against the wrong environment.
CustomModelFetchDtogaineddirectory,providerId, andenv(the variable name, never a value).CustomModelFetchResultDtogainedenvMissing.KiloBackendProviderSettingsManager.fetch()resolves the credential with precedence: typed API key → env var value viaEnvironmentUtil→ the provider's already-stored key fromstate(directory). The last fallback also removes the retyping requirement for providers authenticated with a plain stored API key, not just an env var.envMissing = trueso the dialog can append a hint naming the variable when the fetch fails or returns no models.Three adjacent defects fixed for the same reason (all in
KiloBackendProviderSettingsManager/KiloCliDataParser):disconnect()guard rejected any provider withsource == "env"before checking whether it was config-backed. The guard now only blocks when there's no config entry to delete.kilo.json, because the config patch omitted the key instead of setting it tonull(config updates are a JSON merge-patch, so omission preserves the old value).All changes are confined to
packages/kilo-jetbrains/(Kilo-owned; nokilocode_changemarkers needed, no sharedpackages/opencode/files touched).Screenshots / Video
N/A — no visual layout change beyond a new one-line hint label under the API key field in edit mode.
How to Test
Manual/local verification
./gradlew typecheckfrompackages/kilo-jetbrains/— clean, no new errors../gradlew :backend:test :frontend:testfrompackages/kilo-jetbrains/— 4178 tests, 0 failures, 0 errors, including 12 new backend tests (MockWebServer-backed: env/typed-key/stored-key precedence, header forwarding,envMissing, disconnect fix, config-clear fix) and 8 new frontend tests (DTO contents sent by the dialog, header forwarding, env-hint error text, hint-label visibility).bun run script/check-opencode-annotations.ts --worktreefrom repo root — confirms no sharedpackages/opencode/files changed.Reviewer test steps
kilo.json— theenvkey should be gone (previously it lingered).Blocked checks and substitute verification
./gradlew runIdeSplitMode) was not run in this environment; the automated backend (MockWebServer) and frontend (fake RPC) tests above exercise the same request/response paths and guard logic that a manual run would exercise.Checklist
Get in Touch