Merge pull request #405 from corbitsdev/cl-5687-5688-5689-5690-onboarding-fixes

Fix onboarding's credential validation and mid-session provider connect

Author: TheGreatAxios

src/config/index.ts

@@ -115,6 +115,20 @@ export type ProviderCatalogEntry = Omit<ProviderSettings, "name" | "contextWindo
   // Set when this entry is an xAI/Grok OAuth profile. It still routes through
   // openai-compatible; the marker only controls token refresh and persistence.
   xaiProfile?: string;
+  // When true this provider is backed by a Bifrost virtual key. Inference
+  // sources for it are built with provider "bifrost" so the adapter can
+  // inject the x-bf-vk header (in addition to Authorization). The flag is
+  // also used to enable /models auto-discovery scoped to the key.
+  bifrostVirtualKey?: boolean;
+  // Anthropic Messages API (x-api-key). Used by first-class Anthropic and by
+  // OpenCode Go models that speak the messages protocol.
+  anthropic?: boolean;
+  // OpenCode Go multi-protocol provider. Per-model routing picks
+  // openai-compatible, openai-responses, or anthropic at source-build time.
+  opencodeGo?: boolean;
+  // False when this credential was persisted without a passing connection
+  // test. See ProviderSettings.verified in settings.ts.
+  verified?: boolean;
 };
 
 // Build the InferenceSource for a Codex OAuth profile. Routes to the
@@ -251,6 +265,9 @@ export type Config = {
   model: string;
   providerName: string;
   keyless?: boolean;
+  // False when the active provider's credential was persisted without a
+  // passing connection test. See ProviderSettings.verified in settings.ts.
+  verified?: boolean;
   cwd: string;
   task: string;
   force: boolean;
@@ -616,6 +633,7 @@ export function catalogEntryAsProviderSettings(entry: ProviderCatalogEntry): Pro
     ...(entry.bifrostVirtualKey === true ? { bifrostVirtualKey: true } : {}),
     ...(entry.anthropic === true ? { anthropic: true } : {}),
     ...(go ? { opencodeGo: true } : {}),
+    ...(entry.verified === false ? { verified: false } : {}),
   };
 }
 
@@ -678,6 +696,7 @@ export function buildProviderCatalog(
         ...(p.bifrostVirtualKey === true ? { bifrostVirtualKey: true } : {}),
         ...(p.anthropic === true ? { anthropic: true } : {}),
         ...(go ? { opencodeGo: true } : {}),
+        ...(p.verified === false ? { verified: false } : {}),
       };
     });
   }

src/config/settings.ts

@@ -43,6 +43,17 @@ export type ProviderSettings = {
   anthropic?: boolean;
   // OpenCode Go multi-protocol provider; per-model adapter selection.
   opencodeGo?: boolean;
+  // False when this credential was persisted without a passing connection
+  // test (e.g. the onboarding "save anyway" bypass). Absent/true means either
+  // the test passed or the provider is exempt from it by design. Read once at
+  // startup to warn the operator instead of surfacing a raw auth error.
+  //
+  // Deliberately defaults to trusted: this field did not exist before it was
+  // introduced, so every provider in an existing settings.json has no value
+  // for it, and that must not retroactively flag every current user's
+  // already-working setup as unverified. Only paths that persist a
+  // credential without testing it write `false` explicitly.
+  verified?: boolean;
 };
 
 // Provider+model identity used by the models-first picker (recent / favorites).
@@ -333,6 +344,7 @@ export type ResolvedProvider = {
   model: string;
   providerName: string;
   keyless?: boolean;
+  verified?: boolean;
 };
 
 const CHAT_COMPLETIONS_SUFFIX = "/chat/completions";
@@ -394,6 +406,7 @@ const ProviderSettingsSchema = type({
   "bifrostVirtualKey?": "boolean",
   "anthropic?": "boolean",
   "opencodeGo?": "boolean",
+  "verified?": "boolean",
 });
 
 const ModelRefSchema = type({
@@ -1113,6 +1126,7 @@ export function resolveProvider(input: ResolveInput): ResolvedProvider {
     apiKey: apiKey ?? "",
     model,
     ...(keyless ? { keyless: true } : {}),
+    ...(selected?.verified === false ? { verified: false } : {}),
   };
 }
 

src/settings.test.ts

@@ -1012,6 +1012,12 @@ describe("recent and favorite model helpers", () => {
     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" });

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 })
+    }
+  })
+})

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<ConnectProviderResult> {
   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() }
     },
   })
 

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[] = []

src/tui-opentui/provider-setup.ts

@@ -605,6 +605,10 @@ const RAMP_TICK_MS = 120
 export async function runProviderSetup(
   config: ProviderSetupConfig,
 ): Promise<boolean> {
+  // 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
+      }
     }
   }
 

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<number> {
@@ -26,69 +19,7 @@ export async function runOnboarding(config: UnconfiguredConfig): Promise<number>
 
   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