Skip to content

Commit f712dda

Browse files
committed
Validate a provider credential before onboarding reports it configured
An empty API key on a key-required preset was written as keyless: true, and resolveProvider skips the missing-key check entirely once keyless is set, so a config that should have failed validation loaded as configured and only broke on the first real send. The submit funnel now rejects an empty key on any preset that isn't genuinely keyless-capable (the custom/manual endpoint), instead of downgrading it. The "save anyway" escape hatch had the same failure shape one step further along: a credential that failed its connection test still got persisted, indistinguishable from a verified one. It's now marked verified: false, and the running session surfaces a one-time startup notice pointing back to setup instead of letting the first send fail with a raw adapter error. Both bugs existed twice over: first-run onboarding and the mid-session "connect a new provider" flow each reimplemented the same submit logic, so a fix to one alone would have left the other silently broken. The submit logic moves into its own module and both callers now share it.
1 parent 34cba6e commit f712dda

8 files changed

Lines changed: 267 additions & 125 deletions

File tree

src/config/index.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,9 @@ export type ProviderCatalogEntry = {
127127
// OpenCode Go multi-protocol provider. Per-model routing picks
128128
// openai-compatible, openai-responses, or anthropic at source-build time.
129129
opencodeGo?: boolean;
130+
// False when this credential was persisted without a passing connection
131+
// test. See ProviderSettings.verified in settings.ts.
132+
verified?: boolean;
130133
};
131134

132135
// Build the InferenceSource for a Codex OAuth profile. Routes to the
@@ -263,6 +266,9 @@ export type Config = {
263266
model: string;
264267
providerName: string;
265268
keyless?: boolean;
269+
// False when the active provider's credential was persisted without a
270+
// passing connection test. See ProviderSettings.verified in settings.ts.
271+
verified?: boolean;
266272
cwd: string;
267273
task: string;
268274
force: boolean;
@@ -628,6 +634,7 @@ export function catalogEntryAsProviderSettings(entry: ProviderCatalogEntry): Pro
628634
...(entry.bifrostVirtualKey === true ? { bifrostVirtualKey: true } : {}),
629635
...(entry.anthropic === true ? { anthropic: true } : {}),
630636
...(go ? { opencodeGo: true } : {}),
637+
...(entry.verified === false ? { verified: false } : {}),
631638
};
632639
}
633640

@@ -690,6 +697,7 @@ export function buildProviderCatalog(
690697
...(p.bifrostVirtualKey === true ? { bifrostVirtualKey: true } : {}),
691698
...(p.anthropic === true ? { anthropic: true } : {}),
692699
...(go ? { opencodeGo: true } : {}),
700+
...(p.verified === false ? { verified: false } : {}),
693701
};
694702
});
695703
}

src/config/settings.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,11 @@ export type ProviderSettings = {
4343
anthropic?: boolean;
4444
// OpenCode Go multi-protocol provider; per-model adapter selection.
4545
opencodeGo?: boolean;
46+
// False when this credential was persisted without a passing connection
47+
// test (e.g. the onboarding "save anyway" bypass). Absent/true means either
48+
// the test passed or the provider is exempt from it by design. Read once at
49+
// startup to warn the operator instead of surfacing a raw auth error.
50+
verified?: boolean;
4651
};
4752

4853
// Provider+model identity used by the models-first picker (recent / favorites).
@@ -333,6 +338,7 @@ export type ResolvedProvider = {
333338
model: string;
334339
providerName: string;
335340
keyless?: boolean;
341+
verified?: boolean;
336342
};
337343

338344
const CHAT_COMPLETIONS_SUFFIX = "/chat/completions";
@@ -394,6 +400,7 @@ const ProviderSettingsSchema = type({
394400
"bifrostVirtualKey?": "boolean",
395401
"anthropic?": "boolean",
396402
"opencodeGo?": "boolean",
403+
"verified?": "boolean",
397404
});
398405

399406
const ModelRefSchema = type({
@@ -1113,6 +1120,7 @@ export function resolveProvider(input: ResolveInput): ResolvedProvider {
11131120
apiKey: apiKey ?? "",
11141121
model,
11151122
...(keyless ? { keyless: true } : {}),
1123+
...(selected?.verified === false ? { verified: false } : {}),
11161124
};
11171125
}
11181126

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
import { describe, test, expect } from "bun:test"
2+
import { mkdtemp, rm } from "node:fs/promises"
3+
import { tmpdir } from "node:os"
4+
import { join } from "node:path"
5+
6+
import { createHarness } from "./harness.js"
7+
import { connectProviderInline } from "./provider-connect.js"
8+
import { loadSettings } from "../config/settings.js"
9+
10+
// The mid-session "connect a new provider" flow shares its persistence and
11+
// validation with first-run onboarding (see provider-setup-submit.ts) — this
12+
// pins that an empty key on a key-required preset is rejected here too,
13+
// rather than silently downgraded to a keyless credential.
14+
describe("connectProviderInline", () => {
15+
test("rejects an empty key on a key-required preset without persisting", async () => {
16+
const dir = await mkdtemp(join(tmpdir(), "provider-connect-"))
17+
const settingsPath = join(dir, "settings.json")
18+
try {
19+
const harness = await createHarness({ width: 80, height: 30 })
20+
const resultPromise = connectProviderInline({
21+
providerId: "openai",
22+
settingsPath,
23+
localSettingsPath: join(dir, "local.json"),
24+
cwd: dir,
25+
existing: null,
26+
createRenderer: async () => harness.renderer,
27+
})
28+
await harness.renderOnce()
29+
30+
// initialProviderId lands directly on the api key step; leave it blank.
31+
harness.pressKey("Enter")
32+
await harness.renderOnce()
33+
// Model step: accept the default.
34+
harness.pressKey("Enter")
35+
await harness.renderOnce()
36+
// The rejection is thrown from the async onSubmit handler.
37+
await new Promise((r) => setTimeout(r, 0))
38+
await harness.renderOnce()
39+
40+
const frame = harness.captureCharFrame()
41+
expect(frame).toContain("requires an api key")
42+
43+
harness.pressKey("Ctrl+C")
44+
const result = await resultPromise
45+
expect(result.connected).toBe(false)
46+
expect(await loadSettings(settingsPath)).toBeNull()
47+
} finally {
48+
await rm(dir, { recursive: true, force: true })
49+
}
50+
})
51+
})

src/tui-opentui/provider-connect.ts

Lines changed: 12 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,8 @@
55
* implemented there) — reused via `initialProviderId`, not reimplemented.
66
*/
77

8-
import {
9-
mergeProviderIntoSettings,
10-
saveGlobalSettings,
11-
saveLocalSettings,
12-
type Settings,
13-
} from "../config/settings.js"
14-
import { validateProviderConnection } from "../provider/validate-connection.js"
8+
import type { Settings } from "../config/settings.js"
9+
import { buildProviderSubmitHandler } from "../tui/provider-setup-submit.js"
1510
import { runProviderSetup, type ProviderSetupConfig } from "./provider-setup.js"
1611

1712
export type ConnectProviderInput = {
@@ -40,58 +35,22 @@ export async function connectProviderInline(
4035
input: ConnectProviderInput,
4136
): Promise<ConnectProviderResult> {
4237
let result: ConnectProviderResult = { connected: false }
38+
const submitProvider = buildProviderSubmitHandler(input.settingsPath, input.existing, input.cwd)
4339

4440
const submitted = await runProviderSetup({
4541
showTelemetryNotice: false,
4642
initialProviderId: input.providerId,
4743
...(input.createRenderer !== undefined ? { createRenderer: input.createRenderer } : {}),
4844
...(input.startLogin !== undefined ? { startLogin: input.startLogin } : {}),
49-
onSubmit: async (values, setPhase, { skipValidation, preset, oauth }) => {
50-
const { name, baseURL, apiKey, model } = values
51-
const providerName = name.trim()
52-
const trimmedBaseURL = baseURL.trim()
53-
const trimmedKey = apiKey.trim()
54-
55-
if (oauth !== undefined) {
56-
setPhase("saving")
57-
const base = input.existing ?? { providers: {} }
58-
await saveGlobalSettings(input.settingsPath, {
59-
...base,
60-
defaultProvider: oauth.providerName,
61-
})
62-
await saveLocalSettings(input.localSettingsPath, {
63-
provider: oauth.providerName,
64-
model: model.trim(),
65-
})
66-
result = { connected: true, providerName: oauth.providerName, model: model.trim() }
67-
return
68-
}
69-
70-
if (!skipValidation && preset?.anthropic !== true) {
71-
const check = await validateProviderConnection({
72-
baseURL: trimmedBaseURL,
73-
apiKey: trimmedKey.length > 0 ? trimmedKey : undefined,
74-
})
75-
if (!check.ok) throw new Error(check.error)
76-
}
77-
78-
setPhase("saving")
79-
const selectedModel = model.trim()
80-
const models =
81-
preset !== undefined && preset.models.includes(selectedModel)
82-
? [...preset.models]
83-
: [selectedModel]
84-
const newProvider = {
85-
baseURL: trimmedBaseURL,
86-
models,
87-
defaultModel: selectedModel,
88-
...(trimmedKey.length > 0 ? { apiKey: trimmedKey } : { keyless: true }),
89-
...(preset?.anthropic === true ? { anthropic: true } : {}),
90-
...(preset?.opencodeGo === true ? { opencodeGo: true } : {}),
91-
}
92-
const merged = mergeProviderIntoSettings(input.existing, providerName, newProvider)
93-
await saveGlobalSettings(input.settingsPath, merged)
94-
result = { connected: true, providerName, model: selectedModel }
45+
onSubmit: async (values, setPhase, opts) => {
46+
// Persistence and validation (empty-key rejection, connection test,
47+
// unverified marking) live in the one funnel every provider-setup exit
48+
// path shares — see buildProviderSubmitHandler.
49+
await submitProvider(values, setPhase, opts)
50+
result =
51+
opts.oauth !== undefined
52+
? { connected: true, providerName: opts.oauth.providerName, model: values.model.trim() }
53+
: { connected: true, providerName: values.name.trim(), model: values.model.trim() }
9554
},
9655
})
9756

src/tui/onboarding.ts

Lines changed: 3 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,8 @@
11
import { runTUI } from "./runner.js";
2+
import { buildProviderSubmitHandler } from "./provider-setup-submit.js";
23
import { loadConfig, type UnconfiguredConfig } from "../config/index.js";
3-
import {
4-
globalSettingsPath,
5-
loadSettings,
6-
localSettingsPath,
7-
mergeProviderIntoSettings,
8-
saveGlobalSettings,
9-
saveLocalSettings,
10-
} from "../config/settings.js";
4+
import { globalSettingsPath, loadSettings } from "../config/settings.js";
115
import { activateHeldTelemetry, telemetryFirstRunPending } from "../telemetry/first-run.js";
12-
import { validateProviderConnection } from "../provider/validate-connection.js";
136
import { runProviderSetup } from "../tui-opentui/provider-setup.js";
147

158
export async function runOnboarding(config: UnconfiguredConfig): Promise<number> {
@@ -26,69 +19,7 @@ export async function runOnboarding(config: UnconfiguredConfig): Promise<number>
2619

2720
const submitted = await runProviderSetup({
2821
showTelemetryNotice,
29-
onSubmit: async (values, setPhase, { skipValidation, preset, oauth }) => {
30-
const { name, baseURL, apiKey, model } = values;
31-
const providerName = name.trim();
32-
const trimmedBaseURL = baseURL.trim();
33-
const trimmedKey = apiKey.trim();
34-
35-
// A signed-in subscription provider has no key to test or store: the
36-
// tokens are already in the home-level auth store, and config load
37-
// projects that store into the provider catalog. Only the selection is
38-
// persisted here — the same two files /model writes when switching.
39-
if (oauth !== undefined) {
40-
setPhase("saving");
41-
const base = existing ?? { providers: {} };
42-
await saveGlobalSettings(settingsPath, {
43-
...base,
44-
defaultProvider: oauth.providerName,
45-
});
46-
await saveLocalSettings(localSettingsPath(config.cwd), {
47-
provider: oauth.providerName,
48-
model: model.trim(),
49-
});
50-
return;
51-
}
52-
53-
// Fail fast on a bad base URL/key here rather than mid-conversation
54-
// during the first real stream request. The operator can bypass the
55-
// check (Ctrl+S) for providers that don't expose /models. Anthropic
56-
// Messages endpoints are exempt: the probe is an OpenAI-compatible GET
57-
// /models with a bearer token, which that surface always rejects.
58-
if (!skipValidation && preset?.anthropic !== true) {
59-
const check = await validateProviderConnection({
60-
baseURL: trimmedBaseURL,
61-
apiKey: trimmedKey.length > 0 ? trimmedKey : undefined,
62-
});
63-
if (!check.ok) {
64-
throw new Error(check.error);
65-
}
66-
}
67-
68-
setPhase("saving");
69-
const selectedModel = model.trim();
70-
// A picked provider seeds its whole catalog so /model has more than the
71-
// one model chosen here; the protocol flags cannot be expressed by the
72-
// four form values and come from the catalog entry.
73-
const models =
74-
preset !== undefined && preset.models.includes(selectedModel)
75-
? [...preset.models]
76-
: [selectedModel];
77-
const newProvider = {
78-
baseURL: trimmedBaseURL,
79-
models,
80-
defaultModel: selectedModel,
81-
...(trimmedKey.length > 0 ? { apiKey: trimmedKey } : { keyless: true }),
82-
...(preset?.anthropic === true ? { anthropic: true } : {}),
83-
...(preset?.opencodeGo === true ? { opencodeGo: true } : {}),
84-
};
85-
// Merge new provider with any pre-existing ones. Single write — the form
86-
// stays open (phase label) until saveGlobalSettings resolves, so the user
87-
// sees confirmation before the screen is cleared. Full-spread merge so
88-
// plugins/pluginPaths/sessionMode/shell/tools survive re-onboarding.
89-
const merged = mergeProviderIntoSettings(existing, providerName, newProvider);
90-
await saveGlobalSettings(settingsPath, merged);
91-
},
22+
onSubmit: buildProviderSubmitHandler(settingsPath, existing, config.cwd),
9223
});
9324

9425
// If the user cancelled (Ctrl+C) onSubmit was never called and settings were
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
import { describe, test, expect, afterEach } from "bun:test";
2+
import { mkdtemp, rm } from "node:fs/promises";
3+
import { tmpdir } from "node:os";
4+
import { join } from "node:path";
5+
6+
import { buildProviderSubmitHandler } from "./provider-setup-submit.js";
7+
import { loadSettings } from "../config/settings.js";
8+
import type { ProviderFormValues, SubmitPhase } from "../tui-opentui/provider-setup.js";
9+
10+
const noopSetPhase = (_phase: SubmitPhase): void => {};
11+
12+
async function withTempSettingsPath(
13+
run: (path: string) => Promise<void>,
14+
): Promise<void> {
15+
const dir = await mkdtemp(join(tmpdir(), "provider-setup-submit-"));
16+
const path = join(dir, "settings.json");
17+
try {
18+
await run(path);
19+
} finally {
20+
await rm(dir, { recursive: true, force: true });
21+
}
22+
}
23+
24+
describe("buildProviderSubmitHandler", () => {
25+
test("rejects an empty key on a key-required preset without persisting", async () => {
26+
await withTempSettingsPath(async (path) => {
27+
const submit = buildProviderSubmitHandler(path, null, "/tmp/cwd");
28+
const values: ProviderFormValues = {
29+
name: "openai",
30+
baseURL: "https://api.openai.com/v1",
31+
apiKey: "",
32+
model: "gpt-5",
33+
};
34+
const preset = { id: "openai", models: ["gpt-5"], anthropic: false, opencodeGo: false };
35+
36+
await expect(
37+
submit(values, noopSetPhase, { skipValidation: false, preset }),
38+
).rejects.toThrow(/api key/i);
39+
40+
expect(await loadSettings(path)).toBeNull();
41+
});
42+
});
43+
44+
test("allows an empty key on the manual/custom path (no preset)", async () => {
45+
await withTempSettingsPath(async (path) => {
46+
const submit = buildProviderSubmitHandler(path, null, "/tmp/cwd");
47+
const values: ProviderFormValues = {
48+
name: "local",
49+
baseURL: "http://localhost:11434/v1",
50+
apiKey: "",
51+
model: "llama3",
52+
};
53+
54+
// skipValidation avoids the live connection probe in this unit test.
55+
await submit(values, noopSetPhase, { skipValidation: true });
56+
57+
const settings = await loadSettings(path);
58+
expect(settings?.providers.local?.keyless).toBe(true);
59+
});
60+
});
61+
62+
test("marks a save-anyway submit as unverified", async () => {
63+
await withTempSettingsPath(async (path) => {
64+
const submit = buildProviderSubmitHandler(path, null, "/tmp/cwd");
65+
const values: ProviderFormValues = {
66+
name: "openai",
67+
baseURL: "https://api.openai.com/v1",
68+
apiKey: "sk-test-fake",
69+
model: "gpt-5",
70+
};
71+
const preset = { id: "openai", models: ["gpt-5"], anthropic: false, opencodeGo: false };
72+
73+
await submit(values, noopSetPhase, { skipValidation: true, preset });
74+
75+
const settings = await loadSettings(path);
76+
expect(settings?.providers.openai?.verified).toBe(false);
77+
});
78+
});
79+
});

0 commit comments

Comments
 (0)