diff --git a/src/config/index.ts b/src/config/index.ts index ee4cd78d7..0b4a6d923 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -115,6 +115,20 @@ export type ProviderCatalogEntry = Omit { expect(s.recentModels?.[0]).toEqual({ provider: "a", model: "x11" }); }); + test("pushRecentModel leaves defaultProvider untouched", () => { + const s: Settings = { providers: firepass.providers, defaultProvider: "firepass" }; + const next = pushRecentModel(s, { provider: "other", model: "m1" }); + expect(next.defaultProvider).toBe("firepass"); + }); + test("toggleFavoriteModel adds and removes", () => { let s: Settings = { providers: firepass.providers }; s = toggleFavoriteModel(s, { provider: "a", model: "m1" }); diff --git a/src/tui-opentui/provider-connect.test.ts b/src/tui-opentui/provider-connect.test.ts new file mode 100644 index 000000000..8bbc153a1 --- /dev/null +++ b/src/tui-opentui/provider-connect.test.ts @@ -0,0 +1,51 @@ +import { describe, test, expect } from "bun:test" +import { mkdtemp, rm } from "node:fs/promises" +import { tmpdir } from "node:os" +import { join } from "node:path" + +import { createHarness } from "./harness.js" +import { connectProviderInline } from "./provider-connect.js" +import { loadSettings } from "../config/settings.js" + +// The mid-session "connect a new provider" flow shares its persistence and +// validation with first-run onboarding (see provider-setup-submit.ts) — this +// pins that an empty key on a key-required preset is rejected here too, +// rather than silently downgraded to a keyless credential. +describe("connectProviderInline", () => { + test("rejects an empty key on a key-required preset without persisting", async () => { + const dir = await mkdtemp(join(tmpdir(), "provider-connect-")) + const settingsPath = join(dir, "settings.json") + try { + const harness = await createHarness({ width: 80, height: 30 }) + const resultPromise = connectProviderInline({ + providerId: "openai", + settingsPath, + localSettingsPath: join(dir, "local.json"), + cwd: dir, + existing: null, + createRenderer: async () => harness.renderer, + }) + await harness.renderOnce() + + // initialProviderId lands directly on the api key step; leave it blank. + harness.pressKey("Enter") + await harness.renderOnce() + // Model step: accept the default. + harness.pressKey("Enter") + await harness.renderOnce() + // The rejection is thrown from the async onSubmit handler. + await new Promise((r) => setTimeout(r, 0)) + await harness.renderOnce() + + const frame = harness.captureCharFrame() + expect(frame).toContain("requires an api key") + + harness.pressKey("Ctrl+C") + const result = await resultPromise + expect(result.connected).toBe(false) + expect(await loadSettings(settingsPath)).toBeNull() + } finally { + await rm(dir, { recursive: true, force: true }) + } + }) +}) diff --git a/src/tui-opentui/provider-connect.ts b/src/tui-opentui/provider-connect.ts index e1e20e497..37a9f32ae 100644 --- a/src/tui-opentui/provider-connect.ts +++ b/src/tui-opentui/provider-connect.ts @@ -5,13 +5,8 @@ * implemented there) — reused via `initialProviderId`, not reimplemented. */ -import { - mergeProviderIntoSettings, - saveGlobalSettings, - saveLocalSettings, - type Settings, -} from "../config/settings.js" -import { validateProviderConnection } from "../provider/validate-connection.js" +import type { Settings } from "../config/settings.js" +import { buildProviderSubmitHandler } from "../tui/provider-setup-submit.js" import { runProviderSetup, type ProviderSetupConfig } from "./provider-setup.js" export type ConnectProviderInput = { @@ -40,58 +35,22 @@ export async function connectProviderInline( input: ConnectProviderInput, ): Promise { let result: ConnectProviderResult = { connected: false } + const submitProvider = buildProviderSubmitHandler(input.settingsPath, input.existing, input.cwd) const submitted = await runProviderSetup({ showTelemetryNotice: false, initialProviderId: input.providerId, ...(input.createRenderer !== undefined ? { createRenderer: input.createRenderer } : {}), ...(input.startLogin !== undefined ? { startLogin: input.startLogin } : {}), - onSubmit: async (values, setPhase, { skipValidation, preset, oauth }) => { - const { name, baseURL, apiKey, model } = values - const providerName = name.trim() - const trimmedBaseURL = baseURL.trim() - const trimmedKey = apiKey.trim() - - if (oauth !== undefined) { - setPhase("saving") - const base = input.existing ?? { providers: {} } - await saveGlobalSettings(input.settingsPath, { - ...base, - defaultProvider: oauth.providerName, - }) - await saveLocalSettings(input.localSettingsPath, { - provider: oauth.providerName, - model: model.trim(), - }) - result = { connected: true, providerName: oauth.providerName, model: model.trim() } - return - } - - if (!skipValidation && preset?.anthropic !== true) { - const check = await validateProviderConnection({ - baseURL: trimmedBaseURL, - apiKey: trimmedKey.length > 0 ? trimmedKey : undefined, - }) - if (!check.ok) throw new Error(check.error) - } - - setPhase("saving") - const selectedModel = model.trim() - const models = - preset !== undefined && preset.models.includes(selectedModel) - ? [...preset.models] - : [selectedModel] - const newProvider = { - baseURL: trimmedBaseURL, - models, - defaultModel: selectedModel, - ...(trimmedKey.length > 0 ? { apiKey: trimmedKey } : { keyless: true }), - ...(preset?.anthropic === true ? { anthropic: true } : {}), - ...(preset?.opencodeGo === true ? { opencodeGo: true } : {}), - } - const merged = mergeProviderIntoSettings(input.existing, providerName, newProvider) - await saveGlobalSettings(input.settingsPath, merged) - result = { connected: true, providerName, model: selectedModel } + onSubmit: async (values, setPhase, opts) => { + // Persistence and validation (empty-key rejection, connection test, + // unverified marking) live in the one funnel every provider-setup exit + // path shares — see buildProviderSubmitHandler. + await submitProvider(values, setPhase, opts) + result = + opts.oauth !== undefined + ? { connected: true, providerName: opts.oauth.providerName, model: values.model.trim() } + : { connected: true, providerName: values.name.trim(), model: values.model.trim() } }, }) diff --git a/src/tui-opentui/provider-setup.test.ts b/src/tui-opentui/provider-setup.test.ts index 0b7413da1..5ee18401c 100644 --- a/src/tui-opentui/provider-setup.test.ts +++ b/src/tui-opentui/provider-setup.test.ts @@ -271,6 +271,19 @@ async function mountLogin(opts: { return { done, harness } } +describe("runProviderSetup renderer ownership", () => { + test("does not destroy a caller-supplied renderer on cancel", async () => { + const { done, harness } = await mountSetup() + harness.pressKey("Ctrl+C") + expect(await done).toBe(false) + + // A caller-owned renderer must still be usable for whatever mounted it + // in the first place (a live session resuming its own UI after a + // mid-session reconnect), not torn down out from under it. + expect(harness.renderer.isDestroyed).toBe(false) + }) +}) + describe("runProviderSetup sign-in", () => { test("a subscription provider signs in in place and persists the selection", async () => { const seen: ProviderFormValues[] = [] diff --git a/src/tui-opentui/provider-setup.ts b/src/tui-opentui/provider-setup.ts index 4ecbfd27a..74008c49a 100644 --- a/src/tui-opentui/provider-setup.ts +++ b/src/tui-opentui/provider-setup.ts @@ -605,6 +605,10 @@ const RAMP_TICK_MS = 120 export async function runProviderSetup( config: ProviderSetupConfig, ): Promise { + // A caller-supplied renderer (a headless test harness, or a live session's + // renderer reused for a mid-session reconnect) is owned by that caller — + // teardown here must not destroy it out from under them. + const externalRenderer = config.createRenderer !== undefined const renderer = config.createRenderer ? await config.createRenderer() : await createCliRenderer({ @@ -1075,10 +1079,12 @@ export async function runProviderSetup( } catch { // already unmounted } - try { - renderer.destroy() - } catch { - // already destroyed + if (!externalRenderer) { + try { + renderer.destroy() + } catch { + // already destroyed + } } } diff --git a/src/tui/onboarding.ts b/src/tui/onboarding.ts index 14bd31691..7536a5687 100644 --- a/src/tui/onboarding.ts +++ b/src/tui/onboarding.ts @@ -1,15 +1,8 @@ import { runTUI } from "./runner.js"; +import { buildProviderSubmitHandler } from "./provider-setup-submit.js"; import { loadConfig, type UnconfiguredConfig } from "../config/index.js"; -import { - globalSettingsPath, - loadSettings, - localSettingsPath, - mergeProviderIntoSettings, - saveGlobalSettings, - saveLocalSettings, -} from "../config/settings.js"; +import { globalSettingsPath, loadSettings } from "../config/settings.js"; import { activateHeldTelemetry, telemetryFirstRunPending } from "../telemetry/first-run.js"; -import { validateProviderConnection } from "../provider/validate-connection.js"; import { runProviderSetup } from "../tui-opentui/provider-setup.js"; export async function runOnboarding(config: UnconfiguredConfig): Promise { @@ -26,69 +19,7 @@ export async function runOnboarding(config: UnconfiguredConfig): Promise const submitted = await runProviderSetup({ showTelemetryNotice, - onSubmit: async (values, setPhase, { skipValidation, preset, oauth }) => { - const { name, baseURL, apiKey, model } = values; - const providerName = name.trim(); - const trimmedBaseURL = baseURL.trim(); - const trimmedKey = apiKey.trim(); - - // A signed-in subscription provider has no key to test or store: the - // tokens are already in the home-level auth store, and config load - // projects that store into the provider catalog. Only the selection is - // persisted here — the same two files /model writes when switching. - if (oauth !== undefined) { - setPhase("saving"); - const base = existing ?? { providers: {} }; - await saveGlobalSettings(settingsPath, { - ...base, - defaultProvider: oauth.providerName, - }); - await saveLocalSettings(localSettingsPath(config.cwd), { - provider: oauth.providerName, - model: model.trim(), - }); - return; - } - - // Fail fast on a bad base URL/key here rather than mid-conversation - // during the first real stream request. The operator can bypass the - // check (Ctrl+S) for providers that don't expose /models. Anthropic - // Messages endpoints are exempt: the probe is an OpenAI-compatible GET - // /models with a bearer token, which that surface always rejects. - if (!skipValidation && preset?.anthropic !== true) { - const check = await validateProviderConnection({ - baseURL: trimmedBaseURL, - apiKey: trimmedKey.length > 0 ? trimmedKey : undefined, - }); - if (!check.ok) { - throw new Error(check.error); - } - } - - setPhase("saving"); - const selectedModel = model.trim(); - // A picked provider seeds its whole catalog so /model has more than the - // one model chosen here; the protocol flags cannot be expressed by the - // four form values and come from the catalog entry. - const models = - preset !== undefined && preset.models.includes(selectedModel) - ? [...preset.models] - : [selectedModel]; - const newProvider = { - baseURL: trimmedBaseURL, - models, - defaultModel: selectedModel, - ...(trimmedKey.length > 0 ? { apiKey: trimmedKey } : { keyless: true }), - ...(preset?.anthropic === true ? { anthropic: true } : {}), - ...(preset?.opencodeGo === true ? { opencodeGo: true } : {}), - }; - // Merge new provider with any pre-existing ones. Single write — the form - // stays open (phase label) until saveGlobalSettings resolves, so the user - // sees confirmation before the screen is cleared. Full-spread merge so - // plugins/pluginPaths/sessionMode/shell/tools survive re-onboarding. - const merged = mergeProviderIntoSettings(existing, providerName, newProvider); - await saveGlobalSettings(settingsPath, merged); - }, + onSubmit: buildProviderSubmitHandler(settingsPath, existing, config.cwd), }); // If the user cancelled (Ctrl+C) onSubmit was never called and settings were diff --git a/src/tui/provider-setup-submit.test.ts b/src/tui/provider-setup-submit.test.ts new file mode 100644 index 000000000..9759b76fb --- /dev/null +++ b/src/tui/provider-setup-submit.test.ts @@ -0,0 +1,79 @@ +import { describe, test, expect, afterEach } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { buildProviderSubmitHandler } from "./provider-setup-submit.js"; +import { loadSettings } from "../config/settings.js"; +import type { ProviderFormValues, SubmitPhase } from "../tui-opentui/provider-setup.js"; + +const noopSetPhase = (_phase: SubmitPhase): void => {}; + +async function withTempSettingsPath( + run: (path: string) => Promise, +): Promise { + const dir = await mkdtemp(join(tmpdir(), "provider-setup-submit-")); + const path = join(dir, "settings.json"); + try { + await run(path); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + +describe("buildProviderSubmitHandler", () => { + test("rejects an empty key on a key-required preset without persisting", async () => { + await withTempSettingsPath(async (path) => { + const submit = buildProviderSubmitHandler(path, null, "/tmp/cwd"); + const values: ProviderFormValues = { + name: "openai", + baseURL: "https://api.openai.com/v1", + apiKey: "", + model: "gpt-5", + }; + const preset = { id: "openai", models: ["gpt-5"], anthropic: false, opencodeGo: false }; + + await expect( + submit(values, noopSetPhase, { skipValidation: false, preset }), + ).rejects.toThrow(/api key/i); + + expect(await loadSettings(path)).toBeNull(); + }); + }); + + test("allows an empty key on the manual/custom path (no preset)", async () => { + await withTempSettingsPath(async (path) => { + const submit = buildProviderSubmitHandler(path, null, "/tmp/cwd"); + const values: ProviderFormValues = { + name: "local", + baseURL: "http://localhost:11434/v1", + apiKey: "", + model: "llama3", + }; + + // skipValidation avoids the live connection probe in this unit test. + await submit(values, noopSetPhase, { skipValidation: true }); + + const settings = await loadSettings(path); + expect(settings?.providers.local?.keyless).toBe(true); + }); + }); + + test("marks a save-anyway submit as unverified", async () => { + await withTempSettingsPath(async (path) => { + const submit = buildProviderSubmitHandler(path, null, "/tmp/cwd"); + const values: ProviderFormValues = { + name: "openai", + baseURL: "https://api.openai.com/v1", + apiKey: "sk-test-fake", + model: "gpt-5", + }; + const preset = { id: "openai", models: ["gpt-5"], anthropic: false, opencodeGo: false }; + + await submit(values, noopSetPhase, { skipValidation: true, preset }); + + const settings = await loadSettings(path); + expect(settings?.providers.openai?.verified).toBe(false); + }); + }); +}); diff --git a/src/tui/provider-setup-submit.ts b/src/tui/provider-setup-submit.ts new file mode 100644 index 000000000..6f0ad6991 --- /dev/null +++ b/src/tui/provider-setup-submit.ts @@ -0,0 +1,108 @@ +import { + localSettingsPath, + mergeProviderIntoSettings, + saveGlobalSettings, + saveLocalSettings, + type Settings, +} from "../config/settings.js"; +import { validateProviderConnection } from "../provider/validate-connection.js"; +import type { ProviderSetupSubmit } from "../tui-opentui/provider-setup.js"; + +/** + * The single write path every provider-setup exit takes, shared by first-run + * onboarding and mid-session "connect a new provider" so a credential is + * validated (or explicitly marked unverified) the same way regardless of + * where the form was opened from. + */ +export function buildProviderSubmitHandler( + settingsPath: string, + existing: Settings | null, + cwd: string, +): ProviderSetupSubmit { + return async (values, setPhase, { skipValidation, preset, oauth }) => { + const { name, baseURL, apiKey, model } = values; + const providerName = name.trim(); + const trimmedBaseURL = baseURL.trim(); + const trimmedKey = apiKey.trim(); + + // A signed-in subscription provider has no key to test or store: the + // tokens are already in the home-level auth store, and config load + // projects that store into the provider catalog. Only the selection is + // persisted here — the same two files /model writes when switching. + // + // Unlike a pasted key, this credential was just issued by the real + // provider's own OAuth server completing a PKCE round-trip, so the + // "unverified" concept the API-key path uses doesn't apply the same way + // — there is no separate probe step to skip. What a completed login + // does not confirm is that the resulting token actually carries API + // scope (vs. e.g. a chat-only subscription), which can still surface as + // a first-send auth error; tracked separately rather than faked here + // with a flag this path has no real signal for. + if (oauth !== undefined) { + setPhase("saving"); + const base = existing ?? { providers: {} }; + await saveGlobalSettings(settingsPath, { + ...base, + defaultProvider: oauth.providerName, + }); + await saveLocalSettings(localSettingsPath(cwd), { + provider: oauth.providerName, + model: model.trim(), + }); + return; + } + + // A known preset (anything but the custom/manual endpoint) always speaks + // to a real provider that requires a key; only the manual path is + // genuinely keyless-capable (e.g. a local OpenAI-compatible runtime). + // Reject an empty key here rather than silently downgrading it to + // `keyless: true` and letting resolveProvider skip the missing-key check + // entirely. + if (preset !== undefined && trimmedKey.length === 0) { + throw new Error(`${providerName || preset.id} requires an API key.`); + } + + // Fail fast on a bad base URL/key here rather than mid-conversation + // during the first real stream request. The operator can bypass the + // check (Ctrl+S) for providers that don't expose /models. Anthropic + // Messages endpoints are exempt: the probe is an OpenAI-compatible GET + // /models with a bearer token, which that surface always rejects. + if (!skipValidation && preset?.anthropic !== true) { + const check = await validateProviderConnection({ + baseURL: trimmedBaseURL, + apiKey: trimmedKey.length > 0 ? trimmedKey : undefined, + }); + if (!check.ok) { + throw new Error(check.error); + } + } + + setPhase("saving"); + const selectedModel = model.trim(); + // A picked provider seeds its whole catalog so /model has more than the + // one model chosen here; the protocol flags cannot be expressed by the + // four form values and come from the catalog entry. + const models = + preset !== undefined && preset.models.includes(selectedModel) + ? [...preset.models] + : [selectedModel]; + const newProvider = { + baseURL: trimmedBaseURL, + models, + defaultModel: selectedModel, + ...(trimmedKey.length > 0 ? { apiKey: trimmedKey } : { keyless: true }), + ...(preset?.anthropic === true ? { anthropic: true } : {}), + ...(preset?.opencodeGo === true ? { opencodeGo: true } : {}), + // "Save anyway" (Ctrl+S) persists a credential the connection test + // never passed. Mark it so the running session can warn on first use + // instead of surfacing a bare adapter error. + ...(skipValidation ? { verified: false } : {}), + }; + // Merge new provider with any pre-existing ones. Single write — the form + // stays open (phase label) until saveGlobalSettings resolves, so the user + // sees confirmation before the screen is cleared. Full-spread merge so + // plugins/pluginPaths/sessionMode/shell/tools survive re-onboarding. + const merged = mergeProviderIntoSettings(existing, providerName, newProvider); + await saveGlobalSettings(settingsPath, merged); + }; +} diff --git a/src/tui/runner.ts b/src/tui/runner.ts index b99819a0b..1a4d4ec6c 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -439,6 +439,13 @@ export async function runTUI(initialConfig: Config): Promise { const startupPluginNotices: string[] = []; const discoveryNotice = formatPluginWarningsSummary(pluginLoadDiag.warnings); if (discoveryNotice !== undefined) startupPluginNotices.push(discoveryNotice); + // Saved through onboarding's "save anyway" bypass without a passing + // connection test — warn now instead of a bare adapter error on first send. + if (config.verified === false) { + startupPluginNotices.push( + `We couldn't confirm your "${config.providerName}" key works. If your first message fails with an auth error, double-check the key.`, + ); + } // Mutable list so trusting a project/path plugin can replace a metadata-only stub // with a fully loaded module without restarting the process. let livePluginModules = pluginModules; @@ -2062,7 +2069,7 @@ export async function runTUI(initialConfig: Config): Promise { const onDisk = (await loadGlobalSettingsWriteBase(trueGlobalSettingsPath)) ?? { providers: {}, }; - const next = pushRecentModel({ ...onDisk, defaultProvider: provider }, ref); + const next = pushRecentModel(onDisk, ref); await saveGlobalSettings(trueGlobalSettingsPath, next); config = { ...config, settings: next }; host.refreshModels(listRecentModels(next), listFavoriteModels(next));