diff --git a/CLAUDE.md b/CLAUDE.md deleted file mode 100644 index f6aa6c02..00000000 --- a/CLAUDE.md +++ /dev/null @@ -1,3 +0,0 @@ -# CLAUDE.md - -@AGENTS.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 120000 index 00000000..47dc3e3d --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/src/agentMode/backends/claude/descriptor.ts b/src/agentMode/backends/claude/descriptor.ts index 2a98d55d..1211d700 100644 --- a/src/agentMode/backends/claude/descriptor.ts +++ b/src/agentMode/backends/claude/descriptor.ts @@ -13,12 +13,13 @@ import { import type { AgentSession } from "@/agentMode/session/AgentSession"; import { MethodUnsupportedError } from "@/agentMode/session/errors"; import { resolveClaudeBinary } from "./claudeBinaryResolver"; -import { agentOriginEnabledWireIds } from "@/agentMode/backends/shared/agentEnabledModels"; +import { agentOriginEnabledModelEntries } from "@/agentMode/backends/shared/agentEnabledModels"; import { ClaudeSdkBackendProcess } from "@/agentMode/sdk/ClaudeSdkBackendProcess"; import { getCachedSdkCatalog, synthesizeEffortConfigOption } from "@/agentMode/sdk/effortOption"; import { buildAgentSystemPrompt } from "@/agentMode/backends/shared/agentSystemPrompt"; import type { BackendConfigOption, + EnabledModelEntry, ModeMapping, ModelSelection, ModelWireCodec, @@ -132,9 +133,11 @@ export const ClaudeBackendDescriptor: BackendDescriptor = { wire: claudeWire, showModelDescriptions: true, - getEnabledBaseModelIds(settings: CopilotSettings): ReadonlySet { + getEnabledModelEntries(settings: CopilotSettings): EnabledModelEntry[] { // All Claude Code models are agent-origin. - return agentOriginEnabledWireIds(settings, "claude", (wireId) => claudeWire.decode(wireId)); + return [ + ...agentOriginEnabledModelEntries(settings, "claude", (wireId) => claudeWire.decode(wireId)), + ]; }, getInstallState(settings: CopilotSettings): InstallState { diff --git a/src/agentMode/backends/codex/descriptor.ts b/src/agentMode/backends/codex/descriptor.ts index 25955880..a84ddb9f 100644 --- a/src/agentMode/backends/codex/descriptor.ts +++ b/src/agentMode/backends/codex/descriptor.ts @@ -10,12 +10,17 @@ import { CodexInstallModal } from "./CodexInstallModal"; import CodexLogo from "./logo.svg"; import { CodexSettingsPanel } from "./CodexSettingsPanel"; import type { AgentSession } from "@/agentMode/session/AgentSession"; -import { agentOriginEnabledWireIds } from "@/agentMode/backends/shared/agentEnabledModels"; +import { agentOriginEnabledModelEntries } from "@/agentMode/backends/shared/agentEnabledModels"; import { binaryPathInstallState, simpleBinaryBackendProcess, } from "@/agentMode/backends/shared/simpleBinaryBackend"; -import type { ModeMapping, ModelSelection, ModelWireCodec } from "@/agentMode/session/types"; +import type { + EnabledModelEntry, + ModeMapping, + ModelSelection, + ModelWireCodec, +} from "@/agentMode/session/types"; import type { BackendDescriptor, BackendProcess, InstallState } from "@/agentMode/session/types"; export const CODEX_BINARY_NAME = "codex-acp"; @@ -61,7 +66,7 @@ const codexWire: ModelWireCodec = { * from the local `codex` CLI login. Auth is CLI-owned (no Copilot-side keys), * so the candidate models come entirely from the CLI's live `availableModels` * (active session or preloader cache); curation is the model-management - * `backends.codex.enabledModels` set surfaced via `getEnabledBaseModelIds`. + * `backends.codex.enabledModels` set surfaced via `getEnabledModelEntries`. * * Effort is surfaced via opencode-style model-id parsing — codex-acp * advertises one model per (base × effort) combination, and we collapse @@ -79,9 +84,11 @@ export const CodexBackendDescriptor: BackendDescriptor = { wire: codexWire, showModelDescriptions: true, - getEnabledBaseModelIds(settings: CopilotSettings): ReadonlySet { + getEnabledModelEntries(settings: CopilotSettings): EnabledModelEntry[] { // All Codex models are agent-origin. - return agentOriginEnabledWireIds(settings, "codex", (wireId) => codexWire.decode(wireId)); + return [ + ...agentOriginEnabledModelEntries(settings, "codex", (wireId) => codexWire.decode(wireId)), + ]; }, /** diff --git a/src/agentMode/backends/opencode/descriptor.ts b/src/agentMode/backends/opencode/descriptor.ts index 1eff5123..5cbe84ae 100644 --- a/src/agentMode/backends/opencode/descriptor.ts +++ b/src/agentMode/backends/opencode/descriptor.ts @@ -12,12 +12,17 @@ import { OPENCODE_PROVIDER_MAP, } from "./OpencodeBackend"; import { computeInstallState, OpencodeBinaryManager } from "./OpencodeBinaryManager"; -import { opencodeEnabledWireIds } from "./opencodeModelResolve"; +import { opencodeEnabledModelEntries } from "./opencodeModelResolve"; import { OpencodeSettingsPanel } from "./OpencodeSettingsPanel"; import { mapNodeArch, mapNodePlatform } from "./platformResolver"; import type { AgentSession } from "@/agentMode/session/AgentSession"; import { simpleBinaryBackendProcess } from "@/agentMode/backends/shared/simpleBinaryBackend"; -import type { ModeMapping, ModelSelection, ModelWireCodec } from "@/agentMode/session/types"; +import type { + EnabledModelEntry, + ModeMapping, + ModelSelection, + ModelWireCodec, +} from "@/agentMode/session/types"; import type { BackendDescriptor, BackendProcess, InstallState } from "@/agentMode/session/types"; /** Config option id OpenCode uses to switch the active agent at runtime. */ @@ -98,8 +103,8 @@ export const OpencodeBackendDescriptor: BackendDescriptor = { restartOnSystemPromptChange: true, wire: opencodeWire, - getEnabledBaseModelIds(settings: CopilotSettings): ReadonlySet { - return opencodeEnabledWireIds(settings); + getEnabledModelEntries(settings: CopilotSettings): EnabledModelEntry[] { + return [...opencodeEnabledModelEntries(settings)]; }, getInstallState(settings: CopilotSettings): InstallState { diff --git a/src/agentMode/backends/opencode/opencodeModelResolve.test.ts b/src/agentMode/backends/opencode/opencodeModelResolve.test.ts index 98cddd56..26238fb6 100644 --- a/src/agentMode/backends/opencode/opencodeModelResolve.test.ts +++ b/src/agentMode/backends/opencode/opencodeModelResolve.test.ts @@ -1,6 +1,6 @@ import type { CopilotSettings } from "@/settings/model"; import type { ConfiguredModel, Provider, ProviderOrigin, ProviderType } from "@/modelManagement"; -import { mapProviderToOpencodeId, opencodeEnabledWireIds } from "./opencodeModelResolve"; +import { mapProviderToOpencodeId, opencodeEnabledModelEntries } from "./opencodeModelResolve"; /** Build a minimal `Provider` row for a given origin + type. */ function makeProvider( @@ -29,7 +29,7 @@ function makeModel(configuredModelId: string, providerId: string, wireId: string /** * Assemble a `CopilotSettings`-shaped object with only the slices - * `opencodeEnabledWireIds` reads. Cast through `unknown` since the resolver + * `opencodeEnabledModelEntries` reads. Cast through `unknown` since the resolver * touches just `backends` / `configuredModels` / `providers`. */ function makeSettings(args: { @@ -85,48 +85,71 @@ describe("mapProviderToOpencodeId", () => { }); }); -describe("opencodeEnabledWireIds", () => { - it("returns the shared frozen empty set when no models are enabled", () => { - const first = opencodeEnabledWireIds(makeSettings({ enabledModels: [] })); - const second = opencodeEnabledWireIds(makeSettings({ enabledModels: [] })); - expect(first.size).toBe(0); - // Referential stability: the same frozen constant on every empty call. - expect(first).toBe(second); +describe("opencodeEnabledModelEntries", () => { + const byokProvider = (overrides: Partial = {}): Provider => ({ + ...makeProvider("p1", { kind: "byok", catalogProviderId: "openrouter" }, "openai-compatible"), + requiresApiKey: true, + apiKeyKeychainId: "kc-1", + ...overrides, }); - it("returns the shared frozen empty set when the opencode backend is absent", () => { - const result = opencodeEnabledWireIds(makeSettings({})); - expect(result.size).toBe(0); - }); - - it("builds `/` wire ids for BYOK models", () => { + it("flags a required-key provider with no key as missing_key", () => { const settings = makeSettings({ enabledModels: ["cm1"], - providers: { p1: makeProvider("p1", { kind: "byok", catalogProviderId: "anthropic" }) }, - configuredModels: [makeModel("cm1", "p1", "claude-sonnet-4-6")], + providers: { p1: byokProvider({ apiKeyKeychainId: null }) }, + configuredModels: [makeModel("cm1", "p1", "qwen/qwen3-max")], }); - const result = opencodeEnabledWireIds(settings); - expect([...result]).toEqual(["anthropic/claude-sonnet-4-6"]); + const [entry] = opencodeEnabledModelEntries(settings); + expect(entry.baseModelId).toBe("openrouter/qwen/qwen3-max"); + expect(entry.credentialState).toBe("missing_key"); }); - it("builds `/` wire ids for copilot-plus models", () => { + it("reports a keyed, never-failed provider as ok with its display name", () => { const settings = makeSettings({ enabledModels: ["cm1"], - providers: { p1: makeProvider("p1", { kind: "copilot-plus" }) }, - configuredModels: [makeModel("cm1", "p1", "copilot-plus-flash")], + providers: { p1: byokProvider() }, + configuredModels: [ + { + configuredModelId: "cm1", + providerId: "p1", + info: { id: "x", displayName: "Big X" }, + configuredAt: 0, + }, + ], }); - const result = opencodeEnabledWireIds(settings); - expect([...result]).toEqual(["copilot-plus/copilot-plus-flash"]); + const [entry] = opencodeEnabledModelEntries(settings); + expect(entry.credentialState).toBe("ok"); + expect(entry.name).toBe("Big X"); }); - it("uses the verbatim info.id for agent-origin models (already full wire form)", () => { + it("treats agent-origin (native) models as ok regardless of key", () => { const settings = makeSettings({ enabledModels: ["cm1"], providers: { p1: makeProvider("p1", { kind: "agent", agentType: "opencode" }) }, configuredModels: [makeModel("cm1", "p1", "opencode/big-pickle")], }); - const result = opencodeEnabledWireIds(settings); - expect([...result]).toEqual(["opencode/big-pickle"]); + const [entry] = opencodeEnabledModelEntries(settings); + expect(entry.baseModelId).toBe("opencode/big-pickle"); + expect(entry.credentialState).toBe("ok"); + }); + + it("returns the shared frozen empty array when nothing is enabled", () => { + const first = opencodeEnabledModelEntries(makeSettings({ enabledModels: [] })); + const second = opencodeEnabledModelEntries(makeSettings({ enabledModels: [] })); + expect(first).toHaveLength(0); + // Referential stability: the same frozen constant on every empty call. + expect(first).toBe(second); + }); + + it("builds the `/` wire base id for copilot-plus models", () => { + const settings = makeSettings({ + enabledModels: ["cm1"], + providers: { p1: makeProvider("p1", { kind: "copilot-plus" }) }, + configuredModels: [makeModel("cm1", "p1", "copilot-plus-flash")], + }); + expect(opencodeEnabledModelEntries(settings)[0].baseModelId).toBe( + "copilot-plus/copilot-plus-flash" + ); }); it("skips models whose provider row is missing", () => { @@ -135,7 +158,7 @@ describe("opencodeEnabledWireIds", () => { providers: {}, configuredModels: [makeModel("cm1", "p1", "claude-sonnet-4-6")], }); - expect(opencodeEnabledWireIds(settings).size).toBe(0); + expect(opencodeEnabledModelEntries(settings)).toHaveLength(0); }); it("skips models whose configured-model row is missing", () => { @@ -144,7 +167,7 @@ describe("opencodeEnabledWireIds", () => { providers: { p1: makeProvider("p1", { kind: "byok", catalogProviderId: "anthropic" }) }, configuredModels: [], }); - expect(opencodeEnabledWireIds(settings).size).toBe(0); + expect(opencodeEnabledModelEntries(settings)).toHaveLength(0); }); it("skips models on unroutable providers (BYOK without catalog id)", () => { @@ -153,31 +176,6 @@ describe("opencodeEnabledWireIds", () => { providers: { p1: makeProvider("p1", { kind: "byok" }, "azure") }, configuredModels: [makeModel("cm1", "p1", "some-azure-model")], }); - expect(opencodeEnabledWireIds(settings).size).toBe(0); - }); - - it("builds `/` wire ids for OpenAI-compatible BYOK models", () => { - const settings = makeSettings({ - enabledModels: ["cm1"], - providers: { p1: makeProvider("p1", { kind: "byok" }, "openai-compatible") }, - configuredModels: [makeModel("cm1", "p1", "llama3.2")], - }); - expect([...opencodeEnabledWireIds(settings)]).toEqual(["p1/llama3.2"]); - }); - - it("mixes BYOK and agent-origin models with the correct wire shapes", () => { - const settings = makeSettings({ - enabledModels: ["cm-byok", "cm-agent"], - providers: { - byok: makeProvider("byok", { kind: "byok", catalogProviderId: "openai" }), - agent: makeProvider("agent", { kind: "agent", agentType: "opencode" }), - }, - configuredModels: [ - makeModel("cm-byok", "byok", "gpt-5"), - makeModel("cm-agent", "agent", "opencode/big-pickle"), - ], - }); - const result = opencodeEnabledWireIds(settings); - expect([...result].sort()).toEqual(["openai/gpt-5", "opencode/big-pickle"].sort()); + expect(opencodeEnabledModelEntries(settings)).toHaveLength(0); }); }); diff --git a/src/agentMode/backends/opencode/opencodeModelResolve.ts b/src/agentMode/backends/opencode/opencodeModelResolve.ts index 8d7fb7a3..4144ff31 100644 --- a/src/agentMode/backends/opencode/opencodeModelResolve.ts +++ b/src/agentMode/backends/opencode/opencodeModelResolve.ts @@ -1,5 +1,7 @@ import type { CopilotSettings } from "@/settings/model"; import type { ConfiguredModel, Provider } from "@/modelManagement"; +import { providerRequiresApiKey } from "@/modelManagement"; +import type { EnabledModelCredentialState, EnabledModelEntry } from "@/agentMode/session/types"; export interface OpencodeProviderMapping { /** The opencode provider id — leading segment of `/`. */ @@ -16,7 +18,7 @@ export interface OpencodeProviderMapping { export const COPILOT_PLUS_OPENCODE_PROVIDER_ID = "copilot-plus"; /** See AGENTS.md → "Referential stability". */ -const EMPTY_WIRE_IDS: ReadonlySet = Object.freeze(new Set()); +const EMPTY_ENABLED_ENTRIES: readonly EnabledModelEntry[] = Object.freeze([]); /** * Map a Copilot `Provider` onto its opencode provider id, or `null` when @@ -52,25 +54,48 @@ export function mapProviderToOpencodeId(provider: Provider): OpencodeProviderMap } /** - * The opencode wire ids for the backend's enabled models, joining - * `backends.opencode.enabledModels` to the configured-model + provider state. - * BYOK / Plus models become `${opencodeProviderId}/${info.id}`; agent-origin - * models use `info.id` verbatim (already the full wire form). Unroutable or - * missing entries are skipped. - * - * Shared by `buildOpencodeConfig` (injection) and the descriptor's picker - * filter so the injected / enabled / shown sets agree. + * The opencode wire base id for one routable configured model + * (`/` for non-native, `info.id` verbatim for agent-hosted + * native). Returns `null` when the provider isn't opencode-routable. */ -export function opencodeEnabledWireIds(settings: CopilotSettings): ReadonlySet { +function opencodeWireBaseId(provider: Provider, configuredModel: ConfiguredModel): string | null { + const mapping = mapProviderToOpencodeId(provider); + if (!mapping) return null; + return mapping.native ? configuredModel.info.id : `${mapping.id}/${configuredModel.info.id}`; +} + +/** + * Credential health for an enabled opencode model, derived purely from the + * persisted provider row (sync — no keychain read). Native (agent-hosted) + * providers carry their own auth, so they're always `ok`. Otherwise a + * required-key provider with no key reads `missing_key`. + */ +function credentialStateFor(provider: Provider, native: boolean): EnabledModelCredentialState { + if (native) return "ok"; + if (providerRequiresApiKey(provider) && !provider.apiKeyKeychainId) return "missing_key"; + return "ok"; +} + +/** + * Enabled opencode models enriched for the chat picker: wire base id, display + * name/description, and per-model credential health. Lets the picker iterate + * the enabled set (not the reported∩enabled intersection) so a model opencode + * dropped for a missing/expired key still appears, flagged. Joins + * `backends.opencode.enabledModels` to the configured-model + provider state + * via `opencodeWireBaseId`; unroutable / missing entries are skipped. + */ +export function opencodeEnabledModelEntries( + settings: CopilotSettings +): readonly EnabledModelEntry[] { const enabledIds = settings.backends.opencode?.enabledModels ?? []; - if (enabledIds.length === 0) return EMPTY_WIRE_IDS; + if (enabledIds.length === 0) return EMPTY_ENABLED_ENTRIES; const modelsById = new Map(); for (const model of settings.configuredModels) { modelsById.set(model.configuredModelId, model); } - const wireIds = new Set(); + const out: EnabledModelEntry[] = []; for (const configuredModelId of enabledIds) { const configuredModel = modelsById.get(configuredModelId); if (!configuredModel) continue; @@ -78,15 +103,14 @@ export function opencodeEnabledWireIds(settings: CopilotSettings): ReadonlySet { - it("returns the shared frozen empty set when nothing is enabled", () => { - const a = agentOriginEnabledWireIds(settingsWith("claude", [], []), "claude", bareDecode); - const b = agentOriginEnabledWireIds(settingsWith("codex", [], []), "codex", suffixDecode); - expect(a.size).toBe(0); +describe("agentOriginEnabledModelEntries", () => { + it("returns the shared frozen empty array when nothing is enabled", () => { + const a = agentOriginEnabledModelEntries(settingsWith("claude", [], []), "claude", bareDecode); + const b = agentOriginEnabledModelEntries(settingsWith("codex", [], []), "codex", suffixDecode); + expect(a).toHaveLength(0); // Frozen empty constant — same reference across calls (referential stability). expect(a).toBe(b); }); - it("claude: maps enabled configured-model ids to their bare info.id baseModelId", () => { + it("claude: maps enabled configured-model ids to their bare info.id baseModelId, all ok", () => { const settings = settingsWith( "claude", ["cm1", "cm2"], [model("cm1", "claude-sonnet-4-5"), model("cm2", "claude-opus-4-1")] ); - const set = agentOriginEnabledWireIds(settings, "claude", bareDecode); - expect([...set].sort()).toEqual(["claude-opus-4-1", "claude-sonnet-4-5"]); + const entries = agentOriginEnabledModelEntries(settings, "claude", bareDecode); + expect(entries.map((e) => e.baseModelId).sort()).toEqual([ + "claude-opus-4-1", + "claude-sonnet-4-5", + ]); + // Agent-native: CLI-owned auth, never a credential flag. + expect(entries.every((e) => e.credentialState === "ok")).toBe(true); + expect(entries[0].name).toBe("claude-sonnet-4-5"); }); it("codex: strips the effort suffix to the base model id", () => { const settings = settingsWith("codex", ["cm1"], [model("cm1", "gpt-5/high")]); - const set = agentOriginEnabledWireIds(settings, "codex", suffixDecode); - expect([...set]).toEqual(["gpt-5"]); + const entries = agentOriginEnabledModelEntries(settings, "codex", suffixDecode); + expect(entries.map((e) => e.baseModelId)).toEqual(["gpt-5"]); }); it("skips enabled ids with no matching configured-model row", () => { const settings = settingsWith("claude", ["cm1", "ghost"], [model("cm1", "claude-sonnet-4-5")]); - const set = agentOriginEnabledWireIds(settings, "claude", bareDecode); - expect([...set]).toEqual(["claude-sonnet-4-5"]); + const entries = agentOriginEnabledModelEntries(settings, "claude", bareDecode); + expect(entries.map((e) => e.baseModelId)).toEqual(["claude-sonnet-4-5"]); }); it("only reads the requested agentType's enabledModels", () => { @@ -76,7 +82,7 @@ describe("agentOriginEnabledWireIds", () => { }, configuredModels: [model("cm1", "claude-sonnet-4-5"), model("cm2", "gpt-5")], } as unknown as CopilotSettings; - const claude = agentOriginEnabledWireIds(settings, "claude", bareDecode); - expect([...claude]).toEqual(["claude-sonnet-4-5"]); + const claude = agentOriginEnabledModelEntries(settings, "claude", bareDecode); + expect(claude.map((e) => e.baseModelId)).toEqual(["claude-sonnet-4-5"]); }); }); diff --git a/src/agentMode/backends/shared/agentEnabledModels.ts b/src/agentMode/backends/shared/agentEnabledModels.ts index ebf4ab64..eab68738 100644 --- a/src/agentMode/backends/shared/agentEnabledModels.ts +++ b/src/agentMode/backends/shared/agentEnabledModels.ts @@ -1,43 +1,48 @@ import type { CopilotSettings } from "@/settings/model"; import type { ConfiguredModel } from "@/modelManagement"; +import type { EnabledModelEntry } from "@/agentMode/session/types"; /** See AGENTS.md → "Referential stability". */ -const EMPTY_WIRE_IDS: ReadonlySet = Object.freeze(new Set()); +const EMPTY_ENABLED_ENTRIES: readonly EnabledModelEntry[] = Object.freeze([]); /** A descriptor's own `wire.decode`, accepted as a parameter so this backend-layer helper needn't import the session-domain codec type. */ export type WireDecode = (wireId: string) => { selection: { baseModelId: string } }; /** - * The wire baseModelIds for a claude / codex backend's enabled models — - * each enabled `ConfiguredModel.info.id` decoded via the descriptor's - * `wire.decode`. Used by the descriptor's `getEnabledBaseModelIds` so the - * enabled and picker-shown sets agree. + * The enabled-model entries for a claude / codex backend — each enabled + * `ConfiguredModel.info.id` decoded via the descriptor's `wire.decode` to the + * baseModelId the picker compares against `ModelEntry.baseModelId`, enriched + * with the model's display name/description. These backends are all + * agent-origin (auth is CLI-owned, no Copilot-side keys), so every entry is + * `credentialState: "ok"`. * * Only for all-agent-origin backends; opencode mixes in BYOK/Plus models and - * uses `opencodeEnabledWireIds` instead. + * uses `opencodeEnabledModelEntries` instead. */ -export function agentOriginEnabledWireIds( +export function agentOriginEnabledModelEntries( settings: CopilotSettings, agentType: "claude" | "codex", wireDecode: WireDecode -): ReadonlySet { +): readonly EnabledModelEntry[] { const enabledIds = settings.backends[agentType]?.enabledModels ?? []; - if (enabledIds.length === 0) return EMPTY_WIRE_IDS; + if (enabledIds.length === 0) return EMPTY_ENABLED_ENTRIES; const modelsById = new Map(); for (const model of settings.configuredModels) { modelsById.set(model.configuredModelId, model); } - const wireIds = new Set(); + const entries: EnabledModelEntry[] = []; for (const configuredModelId of enabledIds) { const configuredModel = modelsById.get(configuredModelId); if (!configuredModel) continue; - // `info.id` is the agent-reported wire id; decode it to the baseModelId - // the picker compares against `ModelEntry.baseModelId`. - wireIds.add(wireDecode(configuredModel.info.id).selection.baseModelId); + entries.push({ + baseModelId: wireDecode(configuredModel.info.id).selection.baseModelId, + name: configuredModel.info.displayName || configuredModel.info.id, + description: configuredModel.info.description, + credentialState: "ok", + }); } - if (wireIds.size === 0) return EMPTY_WIRE_IDS; - return wireIds; + return entries.length === 0 ? EMPTY_ENABLED_ENTRIES : entries; } diff --git a/src/agentMode/index.ts b/src/agentMode/index.ts index 4f7b0ad8..97621b73 100644 --- a/src/agentMode/index.ts +++ b/src/agentMode/index.ts @@ -151,6 +151,12 @@ export function createAgentSessionManager(app: App, plugin: CopilotPlugin): Agen // new spawn picks them up. Without this, a key entered after the // subprocess started never reaches it — opencode keeps making un- // authenticated requests and surfaces them as silent zero-token turns. + // + // A single BYOK save fires several emits in quick succession (provider row → + // API key → enabled models). Coalescing them into one re-probe lives a layer + // down: a running backend folds rapid restarts via the manager's restart + // queue, and a warm preload probe folds them via `preloader.refresh` — both + // re-read the *final* config once the burst settles, so no debounce here. const restartProviderAffected = (reason: string): void => { for (const descriptor of listBackendDescriptors()) { if (!descriptor.restartOnProviderConfigChange) continue; diff --git a/src/agentMode/session/AgentModelPreloader.test.ts b/src/agentMode/session/AgentModelPreloader.test.ts index 96a91697..e2eeff59 100644 --- a/src/agentMode/session/AgentModelPreloader.test.ts +++ b/src/agentMode/session/AgentModelPreloader.test.ts @@ -196,6 +196,50 @@ describe("AgentModelPreloader.takeWarm", () => { expect(preloader.takeWarm("claude-sdk")).toBeNull(); }); + it("refresh re-probes a warm backend against current settings", async () => { + const { descriptor } = buildDescriptor(() => makeMockProc()); + const create = descriptor.createBackendProcess as jest.Mock; + const preloader = new AgentModelPreloader(buildApp(), buildPlugin(), () => descriptor); + + await preloader.preload("claude-sdk"); + expect(create).toHaveBeenCalledTimes(1); + + // A config change after the warm probe settled: refresh drops the warm + // entry and runs a fresh probe. + await preloader.refresh("claude-sdk"); + expect(create).toHaveBeenCalledTimes(2); + }); + + it("refresh returns null when nothing is warm or in flight", async () => { + const { descriptor } = buildDescriptor(() => makeMockProc()); + const preloader = new AgentModelPreloader(buildApp(), buildPlugin(), () => descriptor); + + // Never preloaded → a config change must not spin a probe up from nothing. + expect(preloader.refresh("claude-sdk")).toBeNull(); + }); + + it("coalesces a burst of refreshes into a single trailing re-probe", async () => { + // Models a BYOK save: several config writes call refresh in one synchronous + // burst. The 2nd/3rd land while the 1st's probe is still in flight (runProbe + // awaits proc.start before reading the catalog), so they fold into exactly + // one trailing re-probe against the settled settings — not one per write. + const { descriptor } = buildDescriptor(() => makeMockProc()); + const create = descriptor.createBackendProcess as jest.Mock; + const preloader = new AgentModelPreloader(buildApp(), buildPlugin(), () => descriptor); + + await preloader.preload("claude-sdk"); // probe #1 → something warm + expect(create).toHaveBeenCalledTimes(1); + + const chain = preloader.refresh("claude-sdk"); // drops warm, starts probe #2 + expect(chain).not.toBeNull(); + void preloader.refresh("claude-sdk"); // in-flight → trailing-rerun flag + void preloader.refresh("claude-sdk"); // in-flight → already flagged + await chain; + + // probe #2 (in-flight) + exactly one trailing probe #3 = 3 total. + expect(create).toHaveBeenCalledTimes(3); + }); + it("clearCached shuts down and drops a still-warm proc", async () => { const { descriptor, procHandle } = buildDescriptor(() => makeMockProc()); const preloader = new AgentModelPreloader(buildApp(), buildPlugin(), () => descriptor); diff --git a/src/agentMode/session/AgentModelPreloader.ts b/src/agentMode/session/AgentModelPreloader.ts index 1d38b5fb..15450f1b 100644 --- a/src/agentMode/session/AgentModelPreloader.ts +++ b/src/agentMode/session/AgentModelPreloader.ts @@ -51,6 +51,10 @@ export class AgentModelPreloader { // The active session's `attachModelCacheSync` writes here. private readonly cache = new Map(); private readonly inflight = new Map>(); + // Backends whose in-flight probe baked stale spawn config and must re-probe + // once it settles. Set by `refresh`, drained by the probe chain. Coalesces + // the burst of config writes one BYOK save produces into a single re-probe. + private readonly pendingRefresh = new Set(); private readonly listeners = new Set<() => void>(); // Per-warm-entry exit-listener teardowns. Wired when the warm entry is // recorded so we can clear it if the probe subprocess dies before the @@ -127,13 +131,66 @@ export class AgentModelPreloader { if (this.disposed) return Promise.resolve(); const existing = this.inflight.get(backendId); if (existing) return existing; - const promise = this.runProbe(backendId).finally(() => { + return this.startProbeChain(backendId); + } + + /** + * Re-probe `backendId` against current settings after a spawn-config change + * (a new API key, an enabled-models edit, …). Unlike {@link preload} — whose + * dedupe is right for "ensure a warm proc exists" — a config change may land + * *after* an in-flight probe baked its spawn config, so that probe would + * cache a stale catalog and the picker would flag the freshly-enabled model + * "not offered by agent" until a reload. + * + * A single BYOK save lands several writes (provider row → key → enabled + * models) in a burst, each calling here. The first drops the warm entry and + * starts a fresh probe; the rest just flag a trailing re-run, so exactly one + * more probe runs once the in-flight one finishes — observing the settled + * settings. This coalesces the burst into a single final re-probe without a + * debounce timer. + * + * Returns the probe-chain promise (for preload-status wiring), or `null` when + * nothing is warm or in flight — a config change for a never-probed backend + * must not spin one up. + */ + refresh(backendId: BackendId): Promise | null { + if (this.disposed) return null; + const existing = this.inflight.get(backendId); + if (existing) { + this.pendingRefresh.add(backendId); + return existing; + } + if (this.getCachedBackendState(backendId) === null) return null; + this.clearCached(backendId); + return this.startProbeChain(backendId); + } + + /** + * Track a probe as one in-flight promise so concurrent callers dedupe against + * the whole chain, including any trailing re-runs requested via + * {@link refresh}. + */ + private startProbeChain(backendId: BackendId): Promise { + const promise = this.runProbeChain(backendId).finally(() => { this.inflight.delete(backendId); + this.pendingRefresh.delete(backendId); }); this.inflight.set(backendId, promise); return promise; } + private async runProbeChain(backendId: BackendId): Promise { + let round = 0; + do { + this.pendingRefresh.delete(backendId); + // Later rounds replace a warm entry the prior probe set; drop it first so + // the abandoned subprocess is shut down rather than leaked. + if (round > 0) this.clearCached(backendId); + round += 1; + await this.runProbe(backendId); + } while (this.pendingRefresh.has(backendId) && !this.disposed); + } + subscribe(listener: () => void): () => void { this.listeners.add(listener); return () => this.listeners.delete(listener); @@ -143,6 +200,7 @@ export class AgentModelPreloader { this.disposed = true; this.cache.clear(); this.inflight.clear(); + this.pendingRefresh.clear(); this.listeners.clear(); for (const [backendId, warm] of this.warm) { this.warmExitUnsubs.get(backendId)?.(); diff --git a/src/agentMode/session/AgentSessionManager.test.ts b/src/agentMode/session/AgentSessionManager.test.ts index a51c7a49..038f6755 100644 --- a/src/agentMode/session/AgentSessionManager.test.ts +++ b/src/agentMode/session/AgentSessionManager.test.ts @@ -149,6 +149,7 @@ function buildManager(): AgentSessionManager { const modelPreloader = { getCachedBackendState: jest.fn(() => null), preload: jest.fn(async () => undefined), + refresh: jest.fn(() => null), subscribe: jest.fn(() => () => {}), shutdown: jest.fn(), setCached: jest.fn(), @@ -219,6 +220,7 @@ describe("AgentSessionManager.createSession", () => { const modelPreloader = { getCachedBackendState: jest.fn((id: string) => cache.get(id) ?? null), preload: jest.fn(async () => undefined), + refresh: jest.fn(() => null), subscribe: jest.fn(() => () => {}), shutdown: jest.fn(), setCached: jest.fn((id: string, state: unknown) => { @@ -523,6 +525,123 @@ describe("AgentSessionManager.restartBackend", () => { expect(mockBackendShutdown).not.toHaveBeenCalled(); }); + it("refreshes a warm preload probe when the manager owns no process yet", async () => { + // Repro for the BYOK key-add bug: opencode was only preloaded (warm proc + // held by the preloader, never adopted into a session), so the provider + // restart no-oped and the stale probe's catalog made the picker flag the + // freshly-keyed model "not offered by agent" until a full reload. + const refresh = jest.fn(() => Promise.resolve()); + const modelPreloader = { + getCachedBackendState: jest.fn(() => ({ model: null, mode: null })), + preload: jest.fn(async () => undefined), + refresh, + subscribe: jest.fn(() => () => {}), + shutdown: jest.fn(), + setCached: jest.fn(), + clearCached: jest.fn(), + takeWarm: jest.fn(() => null), + }; + const descriptor = { + ...buildDescriptor(), + getInstallState: jest.fn(() => ({ kind: "ready" })), + } as unknown as BackendDescriptor; + const mgr = new AgentSessionManager( + buildApp(), + buildPlugin() as unknown as ConstructorParameters[1], + { + permissionPrompter: jest.fn(), + resolveDescriptor: (id) => (id === descriptor.id ? descriptor : undefined), + modelPreloader: modelPreloader as unknown as ConstructorParameters< + typeof AgentSessionManager + >[2]["modelPreloader"], + } + ); + + // The preloader owns the clear+re-probe (and its coalescing); the manager + // just delegates to `refresh` and registers the returned probe. + await expect(mgr.restartBackend("opencode", "byok key added")).resolves.toBe(true); + expect(refresh).toHaveBeenCalledWith("opencode"); + // No session was ever created, so no proc to shut down. + expect(mockBackendShutdown).not.toHaveBeenCalled(); + }); + + it("does not refresh a warm probe when nothing was preloaded", async () => { + // Backend is installed, but the preloader has nothing warm/in-flight, so + // `refresh` returns null and the manager must not spin a probe up. + const refresh = jest.fn(() => null); + const modelPreloader = { + getCachedBackendState: jest.fn(() => null), + preload: jest.fn(async () => undefined), + refresh, + subscribe: jest.fn(() => () => {}), + shutdown: jest.fn(), + setCached: jest.fn(), + clearCached: jest.fn(), + takeWarm: jest.fn(() => null), + }; + const descriptor = { + ...buildDescriptor(), + getInstallState: jest.fn(() => ({ kind: "ready" })), + } as unknown as BackendDescriptor; + const mgr = new AgentSessionManager( + buildApp(), + buildPlugin() as unknown as ConstructorParameters[1], + { + permissionPrompter: jest.fn(), + resolveDescriptor: (id) => (id === descriptor.id ? descriptor : undefined), + modelPreloader: modelPreloader as unknown as ConstructorParameters< + typeof AgentSessionManager + >[2]["modelPreloader"], + } + ); + + await expect(mgr.restartBackend("opencode", "byok key added")).resolves.toBe(false); + expect(refresh).toHaveBeenCalledWith("opencode"); + }); + + it("re-probes after restart when no replacement session is created", async () => { + // Proc exists but no active session is on this backend (e.g. the active + // tab is on a different agent). The torn-down proc leaves nothing to + // repopulate the cache, so the manager must re-probe — otherwise the + // picker flags freshly-enabled models "not offered by agent" until reload. + const preload = jest.fn(async () => undefined); + const modelPreloader = { + getCachedBackendState: jest.fn(() => null), + preload, + refresh: jest.fn(() => null), + subscribe: jest.fn(() => () => {}), + shutdown: jest.fn(), + setCached: jest.fn(), + clearCached: jest.fn(), + takeWarm: jest.fn(() => null), + }; + const descriptor = { + ...buildDescriptor(), + getInstallState: jest.fn(() => ({ kind: "ready" })), + } as unknown as BackendDescriptor; + const mgr = new AgentSessionManager( + buildApp(), + buildPlugin() as unknown as ConstructorParameters[1], + { + permissionPrompter: jest.fn(), + resolveDescriptor: (id) => (id === descriptor.id ? descriptor : undefined), + modelPreloader: modelPreloader as unknown as ConstructorParameters< + typeof AgentSessionManager + >[2]["modelPreloader"], + } + ); + + // Create then close a session so the proc stays up with no active session. + const session = await mgr.createSession(); + await mgr.closeSession(session.internalId); + preload.mockClear(); + + await expect(mgr.restartBackend("opencode", "byok save")).resolves.toBe(true); + + expect(mockBackendShutdown).toHaveBeenCalled(); + expect(preload).toHaveBeenCalledWith("opencode"); + }); + it("restarts an idle backend and replaces the active affected session", async () => { const mgr = buildManager(); const first = await mgr.createSession(); diff --git a/src/agentMode/session/AgentSessionManager.ts b/src/agentMode/session/AgentSessionManager.ts index 22a6e694..8225d8f5 100644 --- a/src/agentMode/session/AgentSessionManager.ts +++ b/src/agentMode/session/AgentSessionManager.ts @@ -401,8 +401,12 @@ export class AgentSessionManager { * skill discovery and deny rules, is rebuilt from current settings. If a * session on that backend is busy, the restart is deferred until it is idle. * - * Returns `true` when a running backend was restarted or a restart was - * scheduled; `false` when no backend process exists yet. + * When the manager owns no process yet, a warm probe held by the preloader + * may still carry the pre-change spawn config; we refresh it instead (see + * `refreshWarmProbe`) so the first chat-open doesn't adopt a stale proc. + * + * Returns `true` when a running backend was restarted, a restart was + * scheduled, or a warm probe was refreshed; `false` when nothing exists yet. */ async restartBackend(backendId: BackendId, reason: string): Promise { if (this.disposed) return false; @@ -411,7 +415,7 @@ export class AgentSessionManager { await inflight.catch(() => undefined); } const backend = this.backends.get(backendId); - if (!backend) return false; + if (!backend) return this.refreshWarmProbe(backendId, reason); if (this.hasBusySession(backendId)) { const prev = this.pendingBackendRestarts.get(backendId); this.pendingBackendRestarts.set(backendId, prev ? `${prev}; ${reason}` : reason); @@ -422,6 +426,35 @@ export class AgentSessionManager { return true; } + /** + * Refresh a preloader warm probe when the manager owns no process yet. + * Spawn-time config (provider keys, enabled models, native skills, system + * prompt) is baked into the warm probe when it spawns, so a config change + * made before the first chat-open would otherwise never reach it — the + * first `createSession` adopts the stale warm proc and the picker flags + * freshly-enabled models as "not offered by agent" until a full reload. + * + * Delegates the re-probe to `preloader.refresh`, which coalesces the burst of + * writes a single BYOK save produces (provider row → key → enabled models) + * into one re-probe against the settled settings. No-op (returns `false`) + * when nothing is warm/in-flight for this backend, or it isn't installed. + */ + private refreshWarmProbe(backendId: BackendId, reason: string): boolean { + if (this.disposed) return false; + if (!this.isBackendInstalled(backendId)) return false; + const probe = this.preloader.refresh(backendId); + if (!probe) return false; + logInfo(`[AgentMode] refreshing warm ${backendId} probe: ${reason}`); + this.registerPreload(backendId, probe); + return true; + } + + /** Whether `backendId`'s descriptor reports its binary/runtime as installed. */ + private isBackendInstalled(backendId: BackendId): boolean { + const descriptor = this.opts.resolveDescriptor(backendId); + return descriptor?.getInstallState(getSettings())?.kind === "ready"; + } + /** Cached unified backend state for `backendId`, populated by the model preloader. */ getCachedBackendState(backendId: BackendId): BackendState | null { return this.preloader.getCachedBackendState(backendId); @@ -1161,6 +1194,12 @@ export class AgentSessionManager { new Notice(`${this.resolveDescriptor(backendId).displayName} refreshed.`); if (shouldCreateReplacement && !this.disposed) { await this.createSession(backendId); + } else if (!this.disposed && this.isBackendInstalled(backendId)) { + // No live session will repopulate the cache we just cleared (the + // active tab is on a different backend, or there are no tabs). Re-probe + // so the picker reflects the new spawn config instead of flagging + // freshly-enabled models as "not offered by agent" until a reload. + this.registerPreload(backendId, this.preloader.preload(backendId)); } this.notify(); } finally { diff --git a/src/agentMode/session/descriptor.ts b/src/agentMode/session/descriptor.ts index d7dd7c04..3351f928 100644 --- a/src/agentMode/session/descriptor.ts +++ b/src/agentMode/session/descriptor.ts @@ -7,6 +7,7 @@ import type { BackendConfigOption, BackendId, BackendProcess, + EnabledModelEntry, ModelSelection, ModelWireCodec, ModeMapping, @@ -228,13 +229,19 @@ export interface BackendDescriptor { getProbeSessionId?(settings: CopilotSettings): string | undefined; /** - * Optional: the backend's enabled set as wire baseModelIds, which the chat - * picker matches against the reported catalog. The signature is limited to - * `CopilotSettings` so `session/` stays free of `@/modelManagement` — the - * backend implements the join. `null` opts out: the picker then keeps only - * the active session's selection. + * Optional: the backend's enabled models with display metadata and per-model + * credential health — the single accessor the chat picker drives its section + * from. The picker iterates this set (showing every enabled model, flagging + * those the agent can't serve) rather than a reported∩enabled intersection, + * so a model the agent dropped for a missing/expired key — or one it no + * longer reports at all — is never silently hidden. The signature is limited + * to `CopilotSettings` so `session/` stays free of `@/modelManagement` — the + * backend implements the join. Agent-native backends (claude, codex) report + * `credentialState: "ok"` for every entry; key-bearing BYOK backends + * (opencode) compute real per-model health. `null` opts out: the picker then + * keeps only the active session's selection. */ - getEnabledBaseModelIds?(settings: CopilotSettings): ReadonlySet | null; + getEnabledModelEntries?(settings: CopilotSettings): EnabledModelEntry[] | null; /** * Optional: persist the probe sessionId returned by a successful diff --git a/src/agentMode/session/types.ts b/src/agentMode/session/types.ts index fd515904..96877fef 100644 --- a/src/agentMode/session/types.ts +++ b/src/agentMode/session/types.ts @@ -102,6 +102,31 @@ export interface ModelEntry { effortOptions: EffortOption[]; } +/** + * Credential health of an enabled model, derived from its provider row. + * Drives the agent picker's non-selectable flags so an enabled model is + * never silently hidden: + * - `ok` — usable (or agent-hosted / native). + * - `missing_key` — a required-key provider with no key → "Add API key". + */ +export type EnabledModelCredentialState = "ok" | "missing_key"; + +/** + * One entry in a backend's enabled set, enriched for the picker. Lets the + * picker iterate the *enabled* models (rather than the reported∩enabled + * intersection) so a model the agent dropped for a missing/expired key still + * appears, flagged. `baseModelId` is the backend's wire base id (matched + * against the reported catalog's `ModelEntry.baseModelId`). The signature + * stays limited to `CopilotSettings` on the descriptor so `session/` needn't + * import `@/modelManagement` — the backend computes `credentialState`. + */ +export interface EnabledModelEntry { + baseModelId: string; + name: string; + description?: string; + credentialState: EnabledModelCredentialState; +} + /** * Normalized model selection — the single shape used by both runtime * state (`BackendState.model.current`) and persisted preferences diff --git a/src/agentMode/ui/agentModelPickerHelpers.test.ts b/src/agentMode/ui/agentModelPickerHelpers.test.ts index d1b3acc2..f074f4a3 100644 --- a/src/agentMode/ui/agentModelPickerHelpers.test.ts +++ b/src/agentMode/ui/agentModelPickerHelpers.test.ts @@ -1,12 +1,15 @@ import { + appendBackendSection, buildEffortSibling, buildModelOnChange, buildPickerEntries, resolveActiveDisplayState, } from "./agentModelPickerHelpers"; +import type { ModelSelectorEntry } from "@/components/ui/ModelSelector"; import type { BackendDescriptor, BackendState, + EnabledModelEntry, ModelEntry, ModelState, } from "@/agentMode/session/types"; @@ -228,14 +231,20 @@ describe("buildPickerEntries", () => { expect(entries[0].name).toBe("gpt-5"); }); - it("filters by getEnabledBaseModelIds when the descriptor implements it (new path)", () => { + it("filters to the enabled set via getEnabledModelEntries", () => { const enabled = makeModelEntry("anthropic/claude-sonnet-4-6"); const disabled = makeModelEntry("anthropic/claude-haiku"); - // Opencode-style descriptor exposing the new enabled-set hook. Only the - // first model is enabled; the second must be dropped from the picker. + // Only the first model is enabled; the second must be dropped from the + // picker even though the agent reports it. const opencode = { ...makeDescriptor("opencode"), - getEnabledBaseModelIds: () => new Set(["anthropic/claude-sonnet-4-6"]), + getEnabledModelEntries: () => [ + { + baseModelId: "anthropic/claude-sonnet-4-6", + name: "Sonnet", + credentialState: "ok" as const, + }, + ], } as unknown as BackendDescriptor; const manager = makeManager({ cachedStateById: { @@ -258,13 +267,15 @@ describe("buildPickerEntries", () => { expect(entries.map((e) => e.name)).toEqual(["anthropic/claude-sonnet-4-6"]); }); - it("drops every model when the descriptor has no getEnabledBaseModelIds (no legacy branch) except the kept one", () => { - // With the legacy override branch removed, a descriptor that doesn't - // implement getEnabledBaseModelIds yields an empty enabled set, so only - // keepBaseModelId survives. `dropped` is neither enabled nor kept. + it("drops every model not in the enabled set except the kept one", () => { + // An empty enabled set curates nothing in, so only keepBaseModelId + // survives. `dropped` is neither enabled nor kept. const kept = makeModelEntry("kept-model"); const dropped = makeModelEntry("dropped-model"); - const opencode = makeDescriptor("opencode"); + const opencode = { + ...makeDescriptor("opencode"), + getEnabledModelEntries: () => [], + } as unknown as BackendDescriptor; const manager = makeManager({ cachedStateById: { opencode: { model: makeModelState("kept-model", [kept, dropped]), mode: null }, @@ -293,7 +304,9 @@ describe("buildPickerEntries", () => { }; const codex = { ...makeDescriptor("codex"), - getEnabledBaseModelIds: () => new Set(["gpt-5"]), + getEnabledModelEntries: () => [ + { baseModelId: "gpt-5", name: "GPT-5", credentialState: "ok" as const }, + ], } as unknown as BackendDescriptor; const manager = makeManager({ cachedStateById: { @@ -341,7 +354,7 @@ describe("buildPickerEntries", () => { expect(entries[0]._subtitle).toBe("Opus 4.7 with 1M context"); }); - it("keeps the sticky active model even when getEnabledBaseModelIds excludes it", () => { + it("keeps the sticky active model even when the enabled set excludes it", () => { // The active (sticky) model is no longer in the enabled set, but // keepBaseModelId must preserve it so curation never strands the // running selection. @@ -349,7 +362,7 @@ describe("buildPickerEntries", () => { const opencode = { ...makeDescriptor("opencode"), // Empty enabled set — nothing curated in. - getEnabledBaseModelIds: () => new Set(), + getEnabledModelEntries: () => [], } as unknown as BackendDescriptor; const manager = makeManager({ cachedStateById: { @@ -370,6 +383,86 @@ describe("buildPickerEntries", () => { }); }); +// ---- appendBackendSection (enabled-driven credential flags) ---- + +describe("appendBackendSection — getEnabledModelEntries path", () => { + function opencodeWithEntries(enabled: EnabledModelEntry[]): BackendDescriptor { + return { + ...makeDescriptor("opencode"), + getEnabledModelEntries: () => enabled, + }; + } + + it("flags each enabled model by credential state and 'not offered by agent'", () => { + const enabled: EnabledModelEntry[] = [ + { baseModelId: "openrouter/a", name: "A", credentialState: "missing_key" }, + { baseModelId: "openrouter/c", name: "C", credentialState: "ok" }, + { baseModelId: "openrouter/d", name: "D", credentialState: "ok" }, + ]; + const entries: ModelSelectorEntry[] = []; + appendBackendSection(entries, opencodeWithEntries(enabled), { + // Only `c` is reported by the agent; `d` is keyed+ok but unreported. + backendModels: [makeModelEntry("openrouter/c", "Reported C")], + keepBaseModelId: null, + settings: emptySettings, + }); + const byId = Object.fromEntries(entries.map((e) => [e.name, e])); + expect(byId["openrouter/a"]._disabledReason).toBe("Add API key"); + expect(byId["openrouter/c"]._disabledReason).toBeUndefined(); + expect(byId["openrouter/d"]._disabledReason).toBe("Not offered by agent"); + // Reported metadata enriches the row name when present. + expect(byId["openrouter/c"].displayName).toBe("Reported C"); + }); + + it("flags a stale, unreported agent-native model as 'not offered by agent'", () => { + // claude/codex entries are always credentialState "ok"; an enabled id the + // agent no longer reports renders flagged rather than silently hidden. + const claude = { + ...makeDescriptor("claude"), + getEnabledModelEntries: () => [ + { baseModelId: "opus-4-5", name: "Opus 4.5", credentialState: "ok" as const }, + { baseModelId: "retired-model", name: "Retired", credentialState: "ok" as const }, + ], + } as unknown as BackendDescriptor; + const entries: ModelSelectorEntry[] = []; + appendBackendSection(entries, claude, { + backendModels: [makeModelEntry("opus-4-5", "Opus 4.5")], + keepBaseModelId: null, + settings: emptySettings, + }); + const byId = Object.fromEntries(entries.map((e) => [e.name, e])); + expect(byId["opus-4-5"]._disabledReason).toBeUndefined(); + expect(byId["retired-model"]._disabledReason).toBe("Not offered by agent"); + }); + + it("appends a reported keepBaseModelId not already in the enabled set", () => { + const entries: ModelSelectorEntry[] = []; + appendBackendSection( + entries, + opencodeWithEntries([{ baseModelId: "openrouter/a", name: "A", credentialState: "ok" }]), + { + backendModels: [makeModelEntry("openrouter/a"), makeModelEntry("sticky")], + keepBaseModelId: "sticky", + settings: emptySettings, + } + ); + const names = entries.map((e) => e.name); + expect(names).toContain("sticky"); + expect(entries.find((e) => e.name === "sticky")?._disabledReason).toBeUndefined(); + }); + + it("defers to the loading placeholder during preload (no reported catalog yet)", () => { + const entries: ModelSelectorEntry[] = []; + appendBackendSection( + entries, + opencodeWithEntries([{ baseModelId: "openrouter/a", name: "A", credentialState: "ok" }]), + { backendModels: null, keepBaseModelId: null, settings: emptySettings } + ); + // No flags before the catalog loads — buildPickerEntries shows "Loading…". + expect(entries).toHaveLength(0); + }); +}); + // ---- buildEffortSibling ---- describe("buildEffortSibling", () => { diff --git a/src/agentMode/ui/agentModelPickerHelpers.ts b/src/agentMode/ui/agentModelPickerHelpers.ts index e980a7dc..1e73c35c 100644 --- a/src/agentMode/ui/agentModelPickerHelpers.ts +++ b/src/agentMode/ui/agentModelPickerHelpers.ts @@ -12,6 +12,8 @@ import type { BackendDescriptor, BackendId, BackendState, + EnabledModelCredentialState, + EnabledModelEntry, ModelEntry, ModelState, } from "@/agentMode/session/types"; @@ -53,10 +55,22 @@ export function handlePickerSwitchError(err: unknown, action: "model" | "effort" export const AGENT_PROVIDER = "agent"; /** - * Append one backend's section to the picker: its reported catalog filtered to - * the backend's enabled set (`getEnabledBaseModelIds`). The active session's - * selection is always kept via `keepBaseModelId` so curation never strands it. - * A descriptor returning `null` is treated as an empty enabled set. + * Append one backend's section to the picker. + * + * One policy for every backend, via `getEnabledModelEntries`: iterate the + * backend's *enabled* models, enriched by the reported catalog when present. + * Every enabled model appears — one the agent dropped for a missing key, or + * no longer reports at all, is shown non-selectable with a flag + * (`"Add API key"`, `"Not offered by agent"`, …) rather than silently hidden. + * Agent-native backends (claude, codex) report `credentialState: "ok"`, so + * their only flag is `"Not offered by agent"` for a stale enabled id. + * + * The active session's selection (and an inactive backend's persisted default) + * is always kept via `keepBaseModelId`. A descriptor that implements no + * `getEnabledModelEntries` opts out entirely; `buildPickerEntries` still re-adds + * the active selection if curation stranded it. During preload (no reported + * catalog yet) we defer to the "Loading…" placeholder `buildPickerEntries` + * adds, so we don't flag "Not offered by agent" before the catalog loads. */ export function appendBackendSection( entries: ModelSelectorEntry[], @@ -66,19 +80,76 @@ export function appendBackendSection( backendModels: ReadonlyArray | null; /** baseModelId of the active session — never filtered out. */ keepBaseModelId: string | null; - /** Current settings — read by `getEnabledBaseModelIds`. */ + /** Current settings — read by `getEnabledModelEntries`. */ settings: CopilotSettings; } ): void { + const enabledEntries = descriptor.getEnabledModelEntries?.(ctx.settings) ?? null; + if (!enabledEntries) return; + // Preload still pending → no reported catalog yet; let buildPickerEntries + // render the Loading placeholder instead of flagging prematurely. if (!ctx.backendModels) return; - const enabledSet = descriptor.getEnabledBaseModelIds?.(ctx.settings) ?? null; - const filtered = ctx.backendModels.filter( - (entry) => - enabledSet?.has(entry.baseModelId) === true || entry.baseModelId === ctx.keepBaseModelId + appendFromEnabledEntries( + entries, + descriptor, + enabledEntries, + ctx.backendModels, + ctx.keepBaseModelId ); - for (const m of filtered) { - entries.push(synthesizeAgentEntry(m.baseModelId, m.name, descriptor, m.description)); +} + +/** + * Enabled-set-driven section build. Each enabled model becomes a row; the + * reported catalog enriches its name/description when present. A credential + * issue (missing key) or "agent didn't offer it" sets + * `_disabledReason`, which `ModelSelector` renders as a right-side label on a + * non-selectable row. `keepBaseModelId` (the active/default selection) is + * appended as a selectable row when it's reported but not already in the + * enabled set, so curation never strands the running selection. + */ +function appendFromEnabledEntries( + entries: ModelSelectorEntry[], + descriptor: BackendDescriptor, + enabledEntries: ReadonlyArray, + backendModels: ReadonlyArray, + keepBaseModelId: string | null +): void { + const reportedById = new Map(backendModels.map((m) => [m.baseModelId, m])); + const emitted = new Set(); + for (const enabled of enabledEntries) { + const reported = reportedById.get(enabled.baseModelId); + const name = reported?.name || enabled.name || enabled.baseModelId; + const subtitle = reported?.description ?? enabled.description; + const entry = synthesizeAgentEntry(enabled.baseModelId, name, descriptor, subtitle); + const reason = credentialDisabledReason(enabled.credentialState, !!reported); + if (reason) entry._disabledReason = reason; + entries.push(entry); + emitted.add(enabled.baseModelId); } + + if (keepBaseModelId && !emitted.has(keepBaseModelId)) { + const reported = reportedById.get(keepBaseModelId); + if (reported) { + entries.push( + synthesizeAgentEntry(reported.baseModelId, reported.name, descriptor, reported.description) + ); + } + } +} + +/** + * Map an enabled model's credential state (+ whether the agent reported it) to + * a `_disabledReason`, or `undefined` when the row is selectable. Credential + * problems take precedence over "not offered" — they're the root cause of the + * agent dropping the model. + */ +function credentialDisabledReason( + state: EnabledModelCredentialState, + reported: boolean +): string | undefined { + if (state === "missing_key") return "Add API key"; + if (!reported) return "Not offered by agent"; + return undefined; } export function synthesizeAgentEntry( diff --git a/src/modelManagement/index.ts b/src/modelManagement/index.ts index c5a32ab8..ffeb4bb6 100644 --- a/src/modelManagement/index.ts +++ b/src/modelManagement/index.ts @@ -33,6 +33,7 @@ export type { CatalogDownloadDeps, CatalogRefreshResult } from "./catalog/Catalo export { ProviderRegistry } from "./providers/ProviderRegistry"; export { isSelfHostedProvider, isSelfHostedUrl } from "./providers/isSelfHostedProvider"; +export { providerRequiresApiKey } from "./providers/providerRequiresApiKey"; export { ConfiguredModelRegistry } from "./models/ConfiguredModelRegistry"; export { BackendConfigRegistry } from "./backends/BackendConfigRegistry"; export { ChatModelFactory } from "./chatModel/ChatModelFactory"; diff --git a/src/modelManagement/providers/adapters/openaiCompatibleAdapter.test.ts b/src/modelManagement/providers/adapters/openaiCompatibleAdapter.test.ts index 17c5d751..841d7a7e 100644 --- a/src/modelManagement/providers/adapters/openaiCompatibleAdapter.test.ts +++ b/src/modelManagement/providers/adapters/openaiCompatibleAdapter.test.ts @@ -98,4 +98,43 @@ describe("openaiCompatibleAdapter.verifyCredentials", () => { }); expect(mockVerify).toHaveBeenCalledWith("https://example.test/v1/models", expect.any(Object)); }); + + it("verifies OpenRouter against the auth-gated /key path (public /models would 200)", async () => { + await openaiCompatibleAdapter.verifyCredentials({ + provider: provider({ + baseUrl: "https://openrouter.ai/api/v1", + origin: { kind: "byok", catalogProviderId: "openrouter" }, + }), + apiKey: "sk-or", + extras: {}, + }); + expect(mockVerify).toHaveBeenCalledWith("https://openrouter.ai/api/v1/key", { + Authorization: "Bearer sk-or", + }); + }); + + it("uses /models for non-OpenRouter catalog providers", async () => { + await openaiCompatibleAdapter.verifyCredentials({ + provider: provider({ origin: { kind: "byok", catalogProviderId: "groq" } }), + apiKey: "k", + extras: {}, + }); + expect(mockVerify).toHaveBeenCalledWith("https://api.openai.com/v1/models", expect.any(Object)); + }); + + it("maps a 401 from OpenRouter's /key to invalid_api_key", async () => { + // verifyViaListModels already maps 401/403 → invalid_api_key regardless of + // the path, so the /key probe inherits the right classification. + mockVerify.mockResolvedValue({ ok: false, code: "invalid_api_key", checkedAt: 1 }); + const result = await openaiCompatibleAdapter.verifyCredentials({ + provider: provider({ + baseUrl: "https://openrouter.ai/api/v1", + origin: { kind: "byok", catalogProviderId: "openrouter" }, + }), + apiKey: "sk-wrong", + extras: {}, + }); + expect(result.code).toBe("invalid_api_key"); + expect(mockVerify).toHaveBeenCalledWith("https://openrouter.ai/api/v1/key", expect.any(Object)); + }); }); diff --git a/src/modelManagement/providers/adapters/openaiCompatibleAdapter.ts b/src/modelManagement/providers/adapters/openaiCompatibleAdapter.ts index 84376b6b..730c944b 100644 --- a/src/modelManagement/providers/adapters/openaiCompatibleAdapter.ts +++ b/src/modelManagement/providers/adapters/openaiCompatibleAdapter.ts @@ -13,7 +13,9 @@ * * Verification hits the standard `GET /models` endpoint — implemented * by every public OpenAI-compatible provider and by both local - * runners (Ollama / LMStudio). + * runners (Ollama / LMStudio) — except for providers whose `/models` + * is public (OpenRouter), which verify against an auth-gated path via + * `openaiCompatibleVerifyPath`. */ import { z } from "zod"; @@ -22,6 +24,7 @@ import type { BaseChatModel } from "@langchain/core/language_models/chat_models" import type { VerificationResult } from "@/modelManagement/types/runtime"; import type { AdapterBuildContext, AdapterVerifyContext, ProviderAdapter } from "./ProviderAdapter"; +import { verifyPathForCatalogProviderId } from "./openaiCompatibleVerifyPath"; import { verifyViaListModels } from "./verifyViaListModels"; const extrasSchema = z @@ -69,6 +72,14 @@ export const openaiCompatibleAdapter: ProviderAdapter = { headers["OpenAI-Organization"] = ctx.extras.openAIOrgId; } - return verifyViaListModels(`${base}/models`, headers); + // Most providers gate `/models` behind the key; the few whose `/models` + // is public (OpenRouter) verify against an auth-gated path instead, keyed + // by catalog id. `verifyViaListModels` already maps 401/403 → + // `invalid_api_key`, so the `/key` probe needs no special handling. + const catalogProviderId = + ctx.provider.origin.kind === "byok" ? ctx.provider.origin.catalogProviderId : undefined; + const verifyPath = verifyPathForCatalogProviderId(catalogProviderId); + + return verifyViaListModels(`${base}/${verifyPath}`, headers); }, }; diff --git a/src/modelManagement/providers/adapters/openaiCompatibleVerifyPath.ts b/src/modelManagement/providers/adapters/openaiCompatibleVerifyPath.ts new file mode 100644 index 00000000..6c0593ff --- /dev/null +++ b/src/modelManagement/providers/adapters/openaiCompatibleVerifyPath.ts @@ -0,0 +1,34 @@ +/** + * Per-provider verification path for OpenAI-compatible endpoints. + * + * Most hosted OpenAI-compatible providers (OpenAI, Groq, Mistral, DeepSeek, + * xAI) gate `GET /models` behind the API key, so verify-via-`/models` proves + * the key is valid. OpenRouter is the notable exception: its `/models` is + * public and returns 200 for any (or no) `Authorization`, so a blank/wrong key + * would read as "Verified". OpenRouter exposes `GET /api/v1/key` which 401s on + * a bad key and 200s on a valid one, so we probe `/key` for it instead. + * + * Data-driven by `catalogProviderId` so adding another public-`/models` + * provider is a one-line entry, never an `if (openrouter)` branch. + */ + +/** The path suffix appended to the provider's base URL when verifying. */ +export type VerifyPath = "models" | "key"; + +const DEFAULT_VERIFY_PATH: VerifyPath = "models"; + +/** Catalog provider ids whose `/models` is public → verify against an + * auth-gated endpoint instead. */ +const VERIFY_PATH_BY_CATALOG_ID: Record = { + openrouter: "key", +}; + +/** + * The verification path for a given catalog provider id. Returns `"models"` + * for everything not in the override map (including custom endpoints with no + * catalog id). + */ +export function verifyPathForCatalogProviderId(catalogProviderId: string | undefined): VerifyPath { + if (!catalogProviderId) return DEFAULT_VERIFY_PATH; + return VERIFY_PATH_BY_CATALOG_ID[catalogProviderId] ?? DEFAULT_VERIFY_PATH; +} diff --git a/src/modelManagement/providers/providerRequiresApiKey.test.ts b/src/modelManagement/providers/providerRequiresApiKey.test.ts new file mode 100644 index 00000000..5c5126a8 --- /dev/null +++ b/src/modelManagement/providers/providerRequiresApiKey.test.ts @@ -0,0 +1,35 @@ +import type { Provider } from "@/modelManagement/types/persisted"; +import { providerRequiresApiKey } from "./providerRequiresApiKey"; + +function provider(overrides: Partial = {}): Provider { + return { + providerId: "p1", + providerType: "openai-compatible", + displayName: "P", + origin: { kind: "byok" }, + addedAt: 0, + apiKeyKeychainId: null, + ...overrides, + }; +} + +describe("providerRequiresApiKey", () => { + it("returns the explicit flag — the only runtime criteria", () => { + // The flag wins regardless of identity: a hosted catalog provider marked + // keyless reads keyless; a self-hosted row marked key-requiring reads so. + expect( + providerRequiresApiKey( + provider({ requiresApiKey: false, origin: { kind: "byok", catalogProviderId: "openai" } }) + ) + ).toBe(false); + expect( + providerRequiresApiKey(provider({ requiresApiKey: true, baseUrl: "http://localhost:11434" })) + ).toBe(true); + }); + + it("defaults a flagless row to key-requiring (defensive backstop)", () => { + // Post-migration every persisted row carries the flag; a stray undefined + // must never read as keyless or its models would be silently dropped. + expect(providerRequiresApiKey(provider({ requiresApiKey: undefined }))).toBe(true); + }); +}); diff --git a/src/modelManagement/providers/providerRequiresApiKey.ts b/src/modelManagement/providers/providerRequiresApiKey.ts new file mode 100644 index 00000000..5ec2aca7 --- /dev/null +++ b/src/modelManagement/providers/providerRequiresApiKey.ts @@ -0,0 +1,18 @@ +import type { Provider } from "@/modelManagement/types/persisted"; + +/** + * The single runtime read point for "does this provider need an API key". + * + * Reads the explicit, persisted `Provider.requiresApiKey` flag — written at + * every creation path (BYOK setup, agent setup, Plus sign-in) and backfilled + * onto legacy rows by the settings-v5 migration. Self-hosting is NOT the + * criteria — a self-hosted endpoint can sit behind an auth proxy, and a hosted + * provider can be keyless — so the answer is always the stored flag. + * + * The `?? true` is a defensive backstop only: post-migration every persisted + * row carries the flag, but a stray flagless row defaults to key-requiring so a + * key-gated provider is never silently treated as keyless. + */ +export function providerRequiresApiKey(provider: Provider): boolean { + return provider.requiresApiKey ?? true; +} diff --git a/src/modelManagement/setup/AgentSetupApi.ts b/src/modelManagement/setup/AgentSetupApi.ts index c0307353..f475988d 100644 --- a/src/modelManagement/setup/AgentSetupApi.ts +++ b/src/modelManagement/setup/AgentSetupApi.ts @@ -119,6 +119,9 @@ export class AgentSetupApi { baseUrl: input.baseUrl, origin: { kind: "agent", agentType: input.agentType }, extras: input.extras, + // Agent-owned providers route through the agent's own auth (native / + // CLI-managed); the user never supplies a BYOK key for them. + requiresApiKey: false, }); } diff --git a/src/modelManagement/setup/ByokSetupApi.ts b/src/modelManagement/setup/ByokSetupApi.ts index 14687712..8f875105 100644 --- a/src/modelManagement/setup/ByokSetupApi.ts +++ b/src/modelManagement/setup/ByokSetupApi.ts @@ -59,6 +59,11 @@ export interface SetupProviderInput { /** Per-providerType payload (Azure deployment, Bedrock region, * OpenAI org id, …). */ extras?: Record; + /** Whether this provider needs an API key. Flows through from the + * `ProviderDefinition` (`state.source.requiresApiKey`); persisted on the + * `Provider` row so the requires-key question never re-infers from the + * endpoint. Defaults to `true` when omitted (a hosted provider). */ + requiresApiKey?: boolean; /** Full `ModelInfo` snapshots for every model the user selected. * The caller is responsible for catalog enrichment so the API can * stay catalog-agnostic. */ @@ -103,6 +108,9 @@ export class ByokSetupApi { providerType: input.providerType, displayName: input.displayName, baseUrl: input.baseUrl, + // Persist the explicit requires-key flag (default hosted = needs a key) + // so the runtime never re-infers it from the endpoint. + requiresApiKey: input.requiresApiKey ?? true, origin: { kind: "byok", ...(input.catalogProviderId ? { catalogProviderId: input.catalogProviderId } : {}), diff --git a/src/modelManagement/setup/CopilotPlusSetupApi.ts b/src/modelManagement/setup/CopilotPlusSetupApi.ts index 4870c309..6e1a2c77 100644 --- a/src/modelManagement/setup/CopilotPlusSetupApi.ts +++ b/src/modelManagement/setup/CopilotPlusSetupApi.ts @@ -103,6 +103,8 @@ export class CopilotPlusSetupApi { displayName: input.displayName, baseUrl: input.baseUrl, origin: { kind: "copilot-plus" }, + // Provisioned by Plus sign-in, not a user-entered BYOK key. + requiresApiKey: false, }); } diff --git a/src/modelManagement/types/persisted.ts b/src/modelManagement/types/persisted.ts index f72990c6..4f2040c6 100644 --- a/src/modelManagement/types/persisted.ts +++ b/src/modelManagement/types/persisted.ts @@ -81,6 +81,15 @@ export interface Provider { * API key (Ollama, LMStudio, some agent-owned providers). */ apiKeyKeychainId?: string | null; + /** + * Whether this provider needs an API key to function. Authoritative and + * explicit — never inferred from self-hosting at runtime (self-hosted ≠ + * keyless; see `providerRequiresApiKey`). Written at every creation path + * (BYOK setup, agent setup, Plus sign-in). Optional only for rows that + * predate the flag; the settings-v5 migration backfills those by identity, so + * at runtime `providerRequiresApiKey` reads the flag directly. + */ + requiresApiKey?: boolean; /** * Opaque per-`providerType` payload. * azure: { azureDeploymentName, azureApiVersion, azureInstanceName } diff --git a/src/modelManagement/ui/dialogs/ConfigureProviderDialog.test.tsx b/src/modelManagement/ui/dialogs/ConfigureProviderDialog.test.tsx index b91def06..1663075d 100644 --- a/src/modelManagement/ui/dialogs/ConfigureProviderDialog.test.tsx +++ b/src/modelManagement/ui/dialogs/ConfigureProviderDialog.test.tsx @@ -214,10 +214,13 @@ describe("ConfigureProviderForm (new mode)", () => { it("calls setupProvider with the catalog id + enriched model metadata", async () => { mockGetProvider.mockReturnValue(anthropicCatalogMetadata); + mockVerifyCredentials.mockResolvedValue({ ok: true, checkedAt: 1 }); const onClose = jest.fn(); render( ); + // A required-key provider can't save without a key, so supply one. + fireEvent.change(screen.getByTestId("api-key"), { target: { value: "sk-ant" } }); manualAddId("claude-sonnet"); fireEvent.click(screen.getByRole("button", { name: "Save" })); await waitFor(() => expect(mockSetupProvider).toHaveBeenCalledTimes(1)); @@ -345,7 +348,8 @@ describe("ConfigureProviderForm (edit mode)", () => { render( ); - fireEvent.click(screen.getByRole("button", { name: "Test" })); + // The body mounts once the gate resolves the saved key. + fireEvent.click(await screen.findByRole("button", { name: "Test" })); await waitFor(() => expect(mockVerifyCredentials).toHaveBeenCalled()); expect(mockGetApiKey).toHaveBeenCalledWith("p1"); expect(mockSetApiKey).not.toHaveBeenCalled(); @@ -356,7 +360,7 @@ describe("ConfigureProviderForm (edit mode)", () => { render( ); - const baseUrlInput = screen.getByPlaceholderText("https://api.anthropic.com"); + const baseUrlInput = await screen.findByPlaceholderText("https://api.anthropic.com"); fireEvent.change(baseUrlInput, { target: { value: "https://proxy.example.com" } }); fireEvent.click(screen.getByRole("button", { name: "Test" })); await waitFor(() => expect(mockVerifyCredentials).toHaveBeenCalled()); @@ -462,14 +466,32 @@ describe("ConfigureProviderForm (edit mode)", () => { await waitFor(() => expect(onClose).toHaveBeenCalled()); }); - it("Clear button wipes the saved API key and closes the dialog", async () => { + it("Clear empties the field, keeps the dialog open, and persists nothing immediately", async () => { mockGetApiKey.mockResolvedValue("saved-secret"); const onClose = jest.fn(); render(); const clear = await screen.findByTestId("api-key-clear"); fireEvent.click(clear); - await waitFor(() => expect(mockClearApiKey).toHaveBeenCalledWith("p1")); - await waitFor(() => expect(onClose).toHaveBeenCalled()); + // Field is wiped, but the keychain isn't touched and the dialog stays open — + // the removal is staged for Save, like every other field. + await waitFor(() => expect(screen.getByTestId("api-key").value).toBe("")); + expect(mockClearApiKey).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + // Nothing left to clear → the button is gone. + expect(screen.queryByTestId("api-key-clear")).toBeNull(); + }); + + it("disables Save after clearing a required-key provider's key", async () => { + mockGetApiKey.mockResolvedValue("saved-secret"); + render( + + ); + // p1 (anthropic, byok + catalogProviderId) requires a key and seeds two + // selected models, so the empty field is the only thing blocking Save. + fireEvent.click(await screen.findByTestId("api-key-clear")); + await waitFor(() => + expect(screen.getByRole("button", { name: "Save" }).hasAttribute("disabled")).toBe(true) + ); }); }); @@ -526,3 +548,99 @@ describe("ConfigureProviderForm (custom-openai source)", () => { expect(screen.getByPlaceholderText("e.g. gpt-5.5")).toBeTruthy(); }); }); + +describe("ConfigureProviderForm (credential verification + save gating)", () => { + it("A1: a required-key provider with an empty field fails Test without probing", async () => { + render( + + ); + fireEvent.click(screen.getByRole("button", { name: "Test" })); + // No network probe — the guard short-circuits so a public /models 200 + // can't read as "Verified". + expect(mockVerifyCredentials).not.toHaveBeenCalled(); + expect(await screen.findByText("Enter an API key to verify this provider.")).toBeTruthy(); + }); + + it("B2: Save is disabled for a required-key provider with no key", () => { + render( + + ); + manualAddId("claude-sonnet"); + expect(screen.getByRole("button", { name: "Save" }).hasAttribute("disabled")).toBe(true); + }); + + it("B2: an untested invalid key auto-verifies on Save, aborts, and blocks further Save", async () => { + mockVerifyCredentials.mockResolvedValue({ + ok: false, + code: "invalid_api_key", + message: "Authentication failed", + checkedAt: 1, + }); + render( + + ); + fireEvent.change(screen.getByTestId("api-key"), { target: { value: "sk-bad" } }); + manualAddId("claude-sonnet"); + // Untested key → Save is enabled, but Save auto-verifies first. + expect(screen.getByRole("button", { name: "Save" }).hasAttribute("disabled")).toBe(false); + fireEvent.click(screen.getByRole("button", { name: "Save" })); + await waitFor(() => expect(mockVerifyCredentials).toHaveBeenCalled()); + expect(mockSetupProvider).not.toHaveBeenCalled(); + // The conclusive failure now disables Save. + await waitFor(() => + expect(screen.getByRole("button", { name: "Save" }).hasAttribute("disabled")).toBe(true) + ); + }); + + it("B2: an inconclusive verification (network) does not block Save", async () => { + mockVerifyCredentials.mockResolvedValue({ + ok: false, + code: "network", + message: "connection refused", + checkedAt: 1, + }); + const onClose = jest.fn(); + render( + + ); + fireEvent.change(screen.getByTestId("api-key"), { target: { value: "sk-maybe" } }); + manualAddId("claude-sonnet"); + fireEvent.click(screen.getByRole("button", { name: "Save" })); + // Offline users aren't stranded — the provider still saves. + await waitFor(() => expect(mockSetupProvider).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(onClose).toHaveBeenCalled()); + }); + + it("B1/B2: edit mode pre-fills the saved key and saves without re-typing or re-writing it", async () => { + mockGetApiKey.mockResolvedValue("saved-secret"); + mockBulkSet.mockResolvedValue(["cm1", "cm2"]); + const onClose = jest.fn(); + render(); + // The field is seeded with the stored key (visible, masked by PasswordInput). + const input = await screen.findByTestId("api-key"); + expect(input.value).toBe("saved-secret"); + fireEvent.click(screen.getByRole("button", { name: "Save" })); + await waitFor(() => expect(onClose).toHaveBeenCalled()); + // Unchanged key → no keychain churn and no re-verification. + expect(mockSetApiKey).not.toHaveBeenCalled(); + expect(mockVerifyCredentials).not.toHaveBeenCalled(); + }); + + it("B2: a keyless provider (requiresApiKey:false) Tests and Saves with an empty field", async () => { + mockVerifyCredentials.mockResolvedValue({ ok: true, checkedAt: 1 }); + const onClose = jest.fn(); + render( + + ); + // Test with an empty field probes the endpoint (no required-key guard). + fireEvent.click(screen.getByRole("button", { name: "Test" })); + await waitFor(() => expect(mockVerifyCredentials).toHaveBeenCalled()); + manualAddId("llama3.2"); + fireEvent.click(screen.getByRole("button", { name: "Save" })); + await waitFor(() => expect(mockSetupProvider).toHaveBeenCalledTimes(1)); + expect(mockSetupProvider).toHaveBeenCalledWith( + expect.objectContaining({ requiresApiKey: false }) + ); + await waitFor(() => expect(onClose).toHaveBeenCalled()); + }); +}); diff --git a/src/modelManagement/ui/dialogs/ConfigureProviderDialog.tsx b/src/modelManagement/ui/dialogs/ConfigureProviderDialog.tsx index 896ecce8..e639c65b 100644 --- a/src/modelManagement/ui/dialogs/ConfigureProviderDialog.tsx +++ b/src/modelManagement/ui/dialogs/ConfigureProviderDialog.tsx @@ -32,6 +32,7 @@ import { useApp } from "@/context"; import { logError } from "@/logger"; import type { ModelManagementApi } from "@/modelManagement/createModelManagement"; import { BYOK_DEFAULT_AUTO_ENROLL } from "@/modelManagement/setup/ByokSetupApi"; +import { providerRequiresApiKey } from "@/modelManagement/providers/providerRequiresApiKey"; import { byokProvidersAtom, configuredModelsAtom } from "@/modelManagement/state/atoms"; import type { ModelInfo, ProviderType } from "@/modelManagement/types/catalog"; import type { ConfiguredModel, Provider } from "@/modelManagement/types/persisted"; @@ -72,12 +73,16 @@ interface ConfigureProviderFormProps { } /** - * Gate: resolve the persisted provider row and only mount the stateful body - * once it has hydrated. Without this, the body's `useState` initializers - * could read an empty `provider` during the atom-load race and lock in blank - * values the user would then unwittingly Save. + * Gate: resolve the persisted provider row AND its saved API key, and only + * mount the stateful body once both have hydrated. Without this, the body's + * `useState` initializers could read an empty `provider` / blank key during + * the atom-load race and lock in values the user would then unwittingly Save. + * Resolving the key here (rather than probing inside the body) lets edit mode + * seed the key field with the real value — so a genuinely keyless provider is + * visibly empty — without an empty→filled flash. */ export const ConfigureProviderForm: React.FC = ({ state, onClose }) => { + const api = useModelManagement(); const byokProviders = useAtomValue(byokProvidersAtom, { store: settingsStore }); const configuredModels = useAtomValue(configuredModelsAtom, { store: settingsStore }); @@ -97,7 +102,37 @@ export const ConfigureProviderForm: React.FC = ({ st [state, configuredModels] ); - if (state.mode === "edit" && !provider) { + // Edit mode: read the stored key once so the body can seed its field with + // it. New mode resolves immediately with no key. A probe failure resolves + // to `null` (treated as keyless) rather than wedging the spinner. + // + // Two primitive states (not one object) so a re-run that resolves the same + // value bails out via React's `Object.is` short-circuit — the + // `useModelManagement()` hook returns a fresh object each render, so an + // object state here would re-render forever. Keyed on the primitive + // `providerId` so new mode never schedules a probe. + const keyProviderId = state.mode === "edit" ? state.providerId : null; + const [initialApiKey, setInitialApiKey] = useState(null); + const [keyResolved, setKeyResolved] = useState(keyProviderId === null); + useEffect(() => { + if (keyProviderId === null) return; + let cancelled = false; + void api.providerRegistry + .getApiKey(keyProviderId) + .then((key) => { + if (cancelled) return; + setInitialApiKey(key); + setKeyResolved(true); + }) + .catch(() => { + if (!cancelled) setKeyResolved(true); + }); + return () => { + cancelled = true; + }; + }, [keyProviderId, api]); + + if (state.mode === "edit" && (!provider || !keyResolved)) { return (
@@ -111,6 +146,7 @@ export const ConfigureProviderForm: React.FC = ({ st onClose={onClose} provider={provider} existingModels={existingModels} + initialApiKey={initialApiKey} /> ); }; @@ -121,6 +157,9 @@ interface ConfigureProviderBodyProps { /** Guaranteed defined in edit mode (the gate waits for it); undefined in new mode. */ provider: Provider | undefined; existingModels: readonly ConfiguredModel[]; + /** Edit mode: the stored API key, resolved by the gate. `null` for a + * keyless provider (or a probe failure). Always `null` in new mode. */ + initialApiKey: string | null; } const ConfigureProviderBody: React.FC = ({ @@ -128,6 +167,7 @@ const ConfigureProviderBody: React.FC = ({ onClose, provider, existingModels, + initialApiKey, }) => { const api = useModelManagement(); const app = useApp(); @@ -153,7 +193,9 @@ const ConfigureProviderBody: React.FC = ({ const [displayName, setDisplayName] = useState(() => state.mode === "new" ? state.source.displayName : (provider?.displayName ?? "") ); - const [apiKey, setApiKey] = useState(""); + // Edit mode seeds with the resolved stored key (plaintext — it's what + // opencode injects; the field masks it via PasswordInput's reveal toggle). + const [apiKey, setApiKey] = useState(() => initialApiKey ?? ""); const [baseUrl, setBaseUrl] = useState(() => state.mode === "edit" ? (provider?.baseUrl ?? "") : "" ); @@ -165,11 +207,11 @@ const ConfigureProviderBody: React.FC = ({ const [saving, setSaving] = useState(false); const [modelQuery, setModelQuery] = useState(""); - const hasSavedKey = useSavedKeyProbe( - state.mode, - state.mode === "edit" ? state.providerId : undefined, - api - ); + // Whether this provider needs a key. New mode reads the picked definition; + // edit mode reads the explicit persisted flag — never inferred from the + // endpoint. Drives the Test guard, the save gate, and the inline hint. + const requiresApiKey = + state.mode === "new" ? state.source.requiresApiKey : providerRequiresApiKey(provider!); const defaultBaseUrl = state.mode === "new" @@ -188,41 +230,61 @@ const ConfigureProviderBody: React.FC = ({ catalogMetadata, apiKey, extras, - requiresApiKey: state.mode === "new" ? state.source.requiresApiKey : false, + requiresApiKey, providerHydrated: state.mode === "edit" ? !!provider : true, api, }); + // Single verification path shared by Test and save-time auto-verify. In new + // mode the synthetic provider carries `catalogProviderId` so the adapter's + // per-provider verify path (e.g. OpenRouter's auth-gated `/key`) resolves — + // without it OpenRouter's public `/models` would 200 on a blank key. + const runVerification = async (): Promise => { + if (state.mode === "new") { + const synthetic: Provider = { + providerId: "test", + providerType: providerType!, + displayName, + baseUrl: effectiveBaseUrl || undefined, + requiresApiKey: state.source.requiresApiKey, + extras, + origin: { + kind: "byok", + ...(state.source.catalogProviderId + ? { catalogProviderId: state.source.catalogProviderId } + : {}), + }, + addedAt: Date.now(), + }; + return api.adapters.verifyCredentials(providerType!, { + provider: synthetic, + apiKey: apiKey || null, + extras: extras ?? {}, + }); + } + return api.adapters.verifyCredentials(providerType!, { + provider: { ...provider!, displayName, baseUrl: effectiveBaseUrl || undefined, extras }, + apiKey: apiKey || null, + extras: extras ?? {}, + }); + }; + const handleTest = async (): Promise => { if (!providerType) return; + // A1: a required-key provider with an empty field cannot read as + // "Verified" — a public `/models` 200 would lie. Fail fast, no probe. + if (requiresApiKey && apiKey.trim().length === 0) { + setVerification({ + ok: false, + code: "missing_api_key", + message: "Enter an API key to verify this provider.", + checkedAt: Date.now(), + }); + return; + } setTesting(true); try { - let result: VerificationResult; - if (state.mode === "new") { - const synthetic: Provider = { - providerId: "test", - providerType, - displayName, - baseUrl: effectiveBaseUrl || undefined, - extras, - origin: { kind: "byok" }, - addedAt: Date.now(), - }; - result = await api.adapters.verifyCredentials(providerType, { - provider: synthetic, - apiKey: apiKey || null, - extras: extras ?? {}, - }); - } else if (provider) { - const key = apiKey || (await api.providerRegistry.getApiKey(state.providerId)); - result = await api.adapters.verifyCredentials(providerType, { - provider: { ...provider, displayName, baseUrl: effectiveBaseUrl || undefined, extras }, - apiKey: key || null, - extras: extras ?? {}, - }); - } else { - return; - } + const result = await runVerification(); setVerification(result); if (result.ok) { // Successful auth — refetch the model list, since the previous @@ -244,12 +306,24 @@ const ConfigureProviderBody: React.FC = ({ if (state.mode !== "new" || !providerType) return; setSaving(true); try { + // B2: auto-verify a present key before persisting so an untested-but- + // invalid key is caught even if the user never clicked Test. Abort on a + // conclusive failure; proceed on `ok` or an inconclusive result (offline + // users aren't stranded). A keyless field skips the probe. + if (apiKey.trim().length > 0) { + const result = await runVerification(); + if (!result.ok && isConclusiveVerificationFailure(result.code)) { + setVerification(result); + return; + } + } await api.setup.byok.setupProvider({ catalogProviderId: state.source.catalogProviderId, providerType, displayName, baseUrl: effectiveBaseUrl || undefined, apiKey: apiKey || undefined, + requiresApiKey: state.source.requiresApiKey, extras, models: pool.buildSelectedModelInfos(), }); @@ -266,9 +340,20 @@ const ConfigureProviderBody: React.FC = ({ if (state.mode !== "edit" || !provider) return; setSaving(true); try { + // B2: re-verify only a *changed* key (unchanged keys skip the probe and + // the keychain re-write). Abort on a conclusive failure. + const keyChanged = apiKey !== (initialApiKey ?? ""); + if (keyChanged && apiKey.trim().length > 0) { + const result = await runVerification(); + if (!result.ok && isConclusiveVerificationFailure(result.code)) { + setVerification(result); + return; + } + } await saveProviderEdit({ providerId: state.providerId, apiKey, + initialApiKey, displayName, effectiveBaseUrl, extras, @@ -286,15 +371,12 @@ const ConfigureProviderBody: React.FC = ({ } }; - const handleClearKey = async (): Promise => { - if (state.mode !== "edit") return; - try { - await api.providerRegistry.clearApiKey(state.providerId); - onClose(); - } catch (err) { - logError("[ConfigureProviderDialog] clearApiKey failed", err); - new Notice("Failed to clear API key. See console for details."); - } + // Stage the clear locally (like every other field); the keychain write is + // deferred to Save via saveProviderEdit. Empties the field so a required-key + // provider lands in the same un-saveable "no key" state as a fresh setup. + const handleClearKey = (): void => { + setApiKey(""); + setVerification(null); }; const handleRemove = (): void => { @@ -321,11 +403,19 @@ const ConfigureProviderBody: React.FC = ({ const headerName = state.mode === "new" ? state.source.displayName : (provider?.displayName ?? displayName); + // B2: a required-key provider with an empty field can't be saved. + const missingRequiredKey = requiresApiKey && apiKey.trim().length === 0; + // B2: a conclusively-failed verification blocks Save. Inconclusive results + // (network / timeout / rate_limited / http_error) do NOT block — offline + // users can still save. A key edit resets `verification` to `null`, so an + // untested key falls through to the save-time auto-verify in the handlers. + const verificationBlocksSave = isConclusiveVerificationFailure(verification?.code); + // The candidate pool only fills with models the endpoint listed (which // requires working credentials) or ones the user explicitly typed, so a - // non-empty selection already implies a usable setup. No need to - // re-gate on base URL or test status. - const canSave = pool.selectedWireIds.size > 0; + // non-empty selection already implies a usable setup. On top of that, gate + // on a present + non-conclusively-invalid key. + const canSave = pool.selectedWireIds.size > 0 && !missingRequiredKey && !verificationBlocksSave; const testFailed = verification?.ok === false; @@ -351,7 +441,9 @@ const ConfigureProviderBody: React.FC = ({ label={ API key - optional + + {requiresApiKey ? "required" : "optional"} + {verification?.ok === true && ( @@ -370,26 +462,28 @@ const ConfigureProviderBody: React.FC = ({ setVerification(null); }} autoDecrypt={false} - placeholder={ - state.mode === "edit" - ? "Enter a new key to replace the saved one" - : "Paste your API key" - } + placeholder={state.mode === "edit" ? "No API key set" : "Paste your API key"} /> - {state.mode === "edit" && hasSavedKey && ( + {state.mode === "edit" && apiKey.length > 0 && ( )}
- {testFailed && ( + {testFailed ? (
{verification?.message || "Verification failed"}
+ ) : ( + missingRequiredKey && ( +
+ An API key is required for this provider. +
+ ) )} @@ -450,38 +544,26 @@ const ConfigureProviderBody: React.FC = ({ ); }; -/** - * Probe the keychain once in edit mode so the "Clear" button can show only - * when there's something to clear. Probe failures are non-fatal — the button - * just won't appear. - */ -function useSavedKeyProbe( - mode: ConfigureState["mode"], - providerId: string | undefined, - api: ModelManagementApi -): boolean { - const [hasSavedKey, setHasSavedKey] = useState(false); - useEffect(() => { - if (mode !== "edit" || !providerId) return; - let cancelled = false; - void api.providerRegistry - .getApiKey(providerId) - .then((key) => { - if (!cancelled) setHasSavedKey(!!key); - }) - .catch(() => { - // non-fatal - }); - return () => { - cancelled = true; - }; - }, [mode, providerId, api]); - return hasSavedKey; +/** Verification codes that conclusively mean "don't save" — as opposed to + * inconclusive results (network / timeout / rate_limited / http_error) that + * shouldn't strand an offline user. */ +const CONCLUSIVE_VERIFICATION_FAILURE_CODES: readonly string[] = [ + "invalid_api_key", + "missing_api_key", + "missing_base_url", +]; + +function isConclusiveVerificationFailure(code: string | undefined): boolean { + return code !== undefined && CONCLUSIVE_VERIFICATION_FAILURE_CODES.includes(code); } interface SaveEditArgs { providerId: string; apiKey: string; + /** The key the field was seeded with — used to detect a real change so an + * unchanged key doesn't churn the keychain (which would emit and trigger a + * spurious opencode restart). */ + initialApiKey: string | null; displayName: string; effectiveBaseUrl: string; extras: Record; @@ -504,6 +586,7 @@ interface SaveEditArgs { async function saveProviderEdit({ providerId, apiKey, + initialApiKey, displayName, effectiveBaseUrl, extras, @@ -512,7 +595,14 @@ async function saveProviderEdit({ selectedInfos, api, }: SaveEditArgs): Promise { - if (apiKey) await api.providerRegistry.setApiKey(providerId, apiKey); + // Touch the keychain only when the key actually changed — re-writing an + // unchanged key would emit and trigger a spurious opencode restart. A key + // cleared to empty drops the keychain entry (only reachable for keyless + // providers, since a required-key provider with an empty field can't Save). + if (apiKey !== (initialApiKey ?? "")) { + if (apiKey.trim().length > 0) await api.providerRegistry.setApiKey(providerId, apiKey); + else await api.providerRegistry.clearApiKey(providerId); + } await api.providerRegistry.update(providerId, { displayName, baseUrl: effectiveBaseUrl || undefined, diff --git a/src/settings/migrations/byokMigration.test.ts b/src/settings/migrations/byokMigration.test.ts index 3f056b81..0124fad8 100644 --- a/src/settings/migrations/byokMigration.test.ts +++ b/src/settings/migrations/byokMigration.test.ts @@ -312,6 +312,8 @@ describe("planByokMigration — local providers (custom URL required)", () => { providerType: "openai-compatible", baseUrl: "http://192.168.1.5:11434/v1", autoEnrollIn: ["chat", "opencode"], + // Local runner → keyless. + requiresApiKey: false, }); const withoutUrl = planByokMigration( @@ -319,6 +321,22 @@ describe("planByokMigration — local providers (custom URL required)", () => { ); expect(withoutUrl).toEqual([]); }); + + it("sets requiresApiKey:true for key-based providers, false for local runners", () => { + const plan = planByokMigration( + settingsWith([ + model({ name: "claude-x", provider: ChatModelProviders.ANTHROPIC, apiKey: "sk-ant" }), + model({ + name: "llama3.2", + provider: ChatModelProviders.OLLAMA, + baseUrl: "http://localhost:11434/v1", + }), + ]) + ); + expect(byCatalog(plan, "anthropic")?.requiresApiKey).toBe(true); + const ollama = plan.find((p) => p.baseUrl?.includes("11434")); + expect(ollama?.requiresApiKey).toBe(false); + }); }); describe("executeByokMigration", () => { diff --git a/src/settings/migrations/byokMigration.ts b/src/settings/migrations/byokMigration.ts index ecdb35df..22d30340 100644 --- a/src/settings/migrations/byokMigration.ts +++ b/src/settings/migrations/byokMigration.ts @@ -256,6 +256,10 @@ export function planByokMigration(settings: CopilotSettings): SetupProviderInput displayName: displayNameFor(model.provider), models: [], autoEnrollIn: mapping.opencodeRoutable ? ENROLL_CHAT_AND_OPENCODE : ENROLL_CHAT_ONLY, + // Local runners (`requiresBaseUrl`) migrate key-less; every other + // legacy mapping is key-based. Persist it explicitly so the runtime + // never re-infers from the endpoint. + requiresApiKey: !mapping.requiresBaseUrl, }; if (mapping.catalogProviderId) input.catalogProviderId = mapping.catalogProviderId; if (baseUrl) input.baseUrl = baseUrl; diff --git a/src/settings/migrations/index.ts b/src/settings/migrations/index.ts index 9cb9ea64..fce4b3ed 100644 --- a/src/settings/migrations/index.ts +++ b/src/settings/migrations/index.ts @@ -4,9 +4,10 @@ * persisted, and before agent/model-discovery init so OpenCode first sees the * migrated backends). * - * The gate is deliberately a plain version comparison — there is a single - * one-time migration today (legacy BYOK → model-management). Promote this to an - * ordered runner only if a second migration ever accrues. + * Ordered runner: each migration is individually version-gated against the + * vault's stored version, so a vault already at an intermediate version only + * runs the migrations newer than it (the legacy BYOK migration never re-runs + * for a v4 vault picking up the v5 backfill). */ import { logInfo } from "@/logger"; @@ -14,6 +15,7 @@ import type { ModelManagementApi } from "@/modelManagement"; import { getSettings, setSettings } from "@/settings/model"; import { executeByokMigration } from "./byokMigration"; +import { planRequiresApiKeyBackfill } from "./requiresApiKeyMigration"; import { CURRENT_SETTINGS_VERSION } from "./version"; export { CURRENT_SETTINGS_VERSION } from "./version"; @@ -27,8 +29,20 @@ export async function runSettingsMigrations(api: ModelManagementApi): Promise= CURRENT_SETTINGS_VERSION) return; logInfo(`[settings-migration] migrating from v${fromVersion} to v${CURRENT_SETTINGS_VERSION}`); - await executeByokMigration(api, getSettings()); - // Bump unconditionally after the migration so a per-provider failure can't + + // v≤4: legacy BYOK models + keys → the model-management data model. + if (fromVersion < 4) { + await executeByokMigration(api, getSettings()); + } + + // v5: backfill the explicit `requiresApiKey` flag onto rows that predate it, + // so the runtime read point no longer needs an identity heuristic. + if (fromVersion < 5) { + const backfilled = planRequiresApiKeyBackfill(getSettings().providers); + if (backfilled) setSettings({ providers: backfilled }); + } + + // Bump unconditionally after the migrations so a per-provider failure can't // wedge the gate into re-running on every load. Persists via the subscriber. setSettings({ settingsVersion: CURRENT_SETTINGS_VERSION }); } diff --git a/src/settings/migrations/requiresApiKeyMigration.test.ts b/src/settings/migrations/requiresApiKeyMigration.test.ts new file mode 100644 index 00000000..585b7ceb --- /dev/null +++ b/src/settings/migrations/requiresApiKeyMigration.test.ts @@ -0,0 +1,71 @@ +import type { Provider } from "@/modelManagement"; +import { planRequiresApiKeyBackfill } from "./requiresApiKeyMigration"; + +function provider(overrides: Partial = {}): Provider { + return { + providerId: "p1", + providerType: "openai-compatible", + displayName: "P", + origin: { kind: "byok" }, + addedAt: 0, + apiKeyKeychainId: null, + ...overrides, + }; +} + +/** Backfilled `requiresApiKey` for a single flagless row, via the planner. */ +function backfilledFlag(overrides: Partial): boolean | undefined { + const next = planRequiresApiKeyBackfill({ p1: provider(overrides) }); + return next?.p1.requiresApiKey; +} + +describe("planRequiresApiKeyBackfill", () => { + it("treats catalog-backed BYOK as key-requiring", () => { + expect(backfilledFlag({ origin: { kind: "byok", catalogProviderId: "anthropic" } })).toBe(true); + }); + + it("treats a self-hosted catalog-less BYOK as keyless", () => { + expect(backfilledFlag({ baseUrl: "http://localhost:11434/v1" })).toBe(false); + expect(backfilledFlag({ baseUrl: "http://192.168.1.9:1234/v1" })).toBe(false); + }); + + it("requires a key for a catalog-less BYOK pointed at a public host", () => { + expect(backfilledFlag({ baseUrl: "https://proxy.example/v1" })).toBe(true); + }); + + it("defaults a catalog-less BYOK with no base URL to key-requiring", () => { + expect(backfilledFlag({ baseUrl: undefined })).toBe(true); + }); + + it("treats agent-owned and Plus providers as keyless (auth managed elsewhere)", () => { + expect(backfilledFlag({ origin: { kind: "agent", agentType: "opencode" } })).toBe(false); + expect(backfilledFlag({ origin: { kind: "copilot-plus" } })).toBe(false); + }); + + it("never overwrites an already-explicit flag", () => { + // A catalog-backed row the heuristic would call key-requiring stays keyless + // when explicitly flagged so. + const next = planRequiresApiKeyBackfill({ + p1: provider({ + requiresApiKey: false, + origin: { kind: "byok", catalogProviderId: "openai" }, + }), + }); + expect(next).toBeNull(); + }); + + it("returns null when there is nothing to backfill (referential stability)", () => { + expect(planRequiresApiKeyBackfill({})).toBeNull(); + expect(planRequiresApiKeyBackfill({ p1: provider({ requiresApiKey: true }) })).toBeNull(); + }); + + it("backfills only flagless rows, leaving flagged rows untouched", () => { + const flagged = provider({ providerId: "p2", requiresApiKey: true }); + const next = planRequiresApiKeyBackfill({ + p1: provider({ origin: { kind: "byok", catalogProviderId: "anthropic" } }), + p2: flagged, + }); + expect(next?.p1.requiresApiKey).toBe(true); + expect(next?.p2).toBe(flagged); // unchanged reference + }); +}); diff --git a/src/settings/migrations/requiresApiKeyMigration.ts b/src/settings/migrations/requiresApiKeyMigration.ts new file mode 100644 index 00000000..5e929376 --- /dev/null +++ b/src/settings/migrations/requiresApiKeyMigration.ts @@ -0,0 +1,65 @@ +/** + * One-time migration (settings v5): stamp an explicit `Provider.requiresApiKey` + * on every row persisted before the flag existed. + * + * "Does this provider need an API key" is an authoritative, persisted flag — + * never inferred from self-hosting at runtime (self-hosted ≠ keyless). Every + * creation path now writes it (BYOK setup, agent setup, Plus sign-in), so the + * only flagless rows are legacy ones. This backfill resolves them by identity + * once, here, so the runtime read point (`providerRequiresApiKey`) can be a + * pure flag read with no heuristic fallback. + * + * This is the ONLY place the identity heuristic lives — relocated from the old + * runtime `legacyResolveRequiresApiKey`. A one-time migration is where a + * heuristic is acceptable; the runtime criteria is always the explicit flag. + */ + +import { isSelfHostedProvider } from "@/modelManagement"; +import type { Provider } from "@/modelManagement"; + +/** + * Resolve `requiresApiKey` for a flagless row by provider identity. Defaults to + * `true` when unknown so a key-gated provider is never silently treated as + * keyless (which would drop its models downstream). + */ +function resolveByIdentity(provider: Provider): boolean { + switch (provider.origin.kind) { + case "agent": + // Agent-owned providers route through the agent's own auth (native / + // CLI-managed); the user never supplies a BYOK key for them. + return false; + case "copilot-plus": + // Provisioned by Plus sign-in, not a user-entered key. + return false; + case "byok": + // Catalog-backed BYOK providers are hosted clouds — all require a key. + if (provider.origin.catalogProviderId) return true; + // Catalog-less BYOK: a self-hosted runner (Ollama / LM Studio / local + // proxy) commonly runs key-less; anything else (custom hosted proxy) + // needs one. + return !isSelfHostedProvider(provider); + default: + return true; + } +} + +/** + * Pure planner: returns a new `providers` map with `requiresApiKey` backfilled + * on every flagless row, or `null` when nothing changed (so the caller can skip + * a redundant write — referential stability, see AGENTS.md). + */ +export function planRequiresApiKeyBackfill( + providers: Record +): Record | null { + let changed = false; + const next: Record = {}; + for (const [id, provider] of Object.entries(providers)) { + if (provider.requiresApiKey === undefined) { + next[id] = { ...provider, requiresApiKey: resolveByIdentity(provider) }; + changed = true; + } else { + next[id] = provider; + } + } + return changed ? next : null; +} diff --git a/src/settings/migrations/runSettingsMigrations.test.ts b/src/settings/migrations/runSettingsMigrations.test.ts index 85132a5a..5e119ddc 100644 --- a/src/settings/migrations/runSettingsMigrations.test.ts +++ b/src/settings/migrations/runSettingsMigrations.test.ts @@ -85,6 +85,35 @@ it("bumps the version even when there is nothing to migrate", async () => { expect(mockSetSettings).toHaveBeenCalledWith({ settingsVersion: CURRENT_SETTINGS_VERSION }); }); +it("runs only the v5 backfill for a v4 vault (legacy BYOK migration does not re-run)", async () => { + mockGetSettings.mockReturnValue( + settings({ + settingsVersion: 4, + providers: { + p1: { + providerId: "p1", + providerType: "openai-compatible", + displayName: "OpenRouter", + origin: { kind: "byok", catalogProviderId: "openrouter" }, + addedAt: 0, + apiKeyKeychainId: null, + }, + }, + }) + ); + const { api, setupProvider } = makeApi(); + + await runSettingsMigrations(api); + + expect(setupProvider).not.toHaveBeenCalled(); + // Backfill writes the flag, then the version bump lands. + const providerWrite = mockSetSettings.mock.calls.find((call) => "providers" in call[0])?.[0] as + | { providers: Record } + | undefined; + expect(providerWrite?.providers.p1.requiresApiKey).toBe(true); + expect(mockSetSettings).toHaveBeenCalledWith({ settingsVersion: CURRENT_SETTINGS_VERSION }); +}); + it("skips when already at the current version", async () => { mockGetSettings.mockReturnValue(settings({ settingsVersion: CURRENT_SETTINGS_VERSION })); const { api, setupProvider } = makeApi(); diff --git a/src/settings/migrations/version.ts b/src/settings/migrations/version.ts index 6be1c38f..68eacd02 100644 --- a/src/settings/migrations/version.ts +++ b/src/settings/migrations/version.ts @@ -5,8 +5,13 @@ * settings-persistence layer can stamp fresh installs without pulling in the * model-management barrel that `runSettingsMigrations` depends on. * - * `= 4` matches the v4 launch. Gate is `(settingsVersion ?? 0) < CURRENT`, so - * pre-versioned installs (real users → `0`) and the orphaned prototype `2` - * both run; migrated vaults (`4`) and freshly-stamped installs skip. + * Gate is `(settingsVersion ?? 0) < CURRENT`, so pre-versioned installs (real + * users → `0`) and the orphaned prototype `2` run every pending migration; + * freshly-stamped installs skip. Each migration is individually version-gated + * in `runSettingsMigrations`, so a vault already at an intermediate version + * only runs the migrations newer than it. + * + * ≤ 4 → legacy BYOK → model-management migration. + * 5 → backfill `Provider.requiresApiKey` on flagless rows. */ -export const CURRENT_SETTINGS_VERSION = 4; +export const CURRENT_SETTINGS_VERSION = 5; diff --git a/src/settings/v2/components/configuredModelGrouping.ts b/src/settings/v2/components/configuredModelGrouping.ts index 8b288902..5dd40587 100644 --- a/src/settings/v2/components/configuredModelGrouping.ts +++ b/src/settings/v2/components/configuredModelGrouping.ts @@ -24,8 +24,8 @@ export interface CandidatePartition { * * For opencode, a BYOK/Plus provider it can't route (`isOpencodeRoutable` * false — azure / bedrock / self-hosted) is dropped to avoid a dead toggle: - * `opencodeEnabledWireIds` skips unroutable providers at injection and - * picker-filter time, so enabling one would never reach the agent or picker. + * `opencodeEnabledModelEntries` skips unroutable providers, so enabling one + * would never reach the agent or picker. */ export function partitionCandidates( configuredModels: readonly ConfiguredModel[],