Skip to content

Commit dd527d5

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 plain-language startup notice 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. verified defaults to trusted (absent, not false) so this doesn't retroactively flag every existing user's already-working setup; only a path that persists a credential without testing it sets it false. The OAuth submit path is exempt from the same marking on purpose — a completed OAuth login is a stronger signal than a pasted key (the provider's own server just issued it), though it doesn't confirm the resulting token carries API scope, which is tracked as a separate follow-up rather than faked here.
1 parent f712dda commit dd527d5

5 files changed

Lines changed: 39 additions & 5 deletions

File tree

src/config/settings.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,12 @@ export type ProviderSettings = {
4747
// test (e.g. the onboarding "save anyway" bypass). Absent/true means either
4848
// the test passed or the provider is exempt from it by design. Read once at
4949
// startup to warn the operator instead of surfacing a raw auth error.
50+
//
51+
// Deliberately defaults to trusted: this field did not exist before it was
52+
// introduced, so every provider in an existing settings.json has no value
53+
// for it, and that must not retroactively flag every current user's
54+
// already-working setup as unverified. Only paths that persist a
55+
// credential without testing it write `false` explicitly.
5056
verified?: boolean;
5157
};
5258

src/tui-opentui/provider-setup.test.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,19 @@ async function mountLogin(opts: {
271271
return { done, harness }
272272
}
273273

274+
describe("runProviderSetup renderer ownership", () => {
275+
test("does not destroy a caller-supplied renderer on cancel", async () => {
276+
const { done, harness } = await mountSetup()
277+
harness.pressKey("Ctrl+C")
278+
expect(await done).toBe(false)
279+
280+
// A caller-owned renderer must still be usable for whatever mounted it
281+
// in the first place (a live session resuming its own UI after a
282+
// mid-session reconnect), not torn down out from under it.
283+
expect(harness.renderer.isDestroyed).toBe(false)
284+
})
285+
})
286+
274287
describe("runProviderSetup sign-in", () => {
275288
test("a subscription provider signs in in place and persists the selection", async () => {
276289
const seen: ProviderFormValues[] = []

src/tui-opentui/provider-setup.ts

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -605,6 +605,10 @@ const RAMP_TICK_MS = 120
605605
export async function runProviderSetup(
606606
config: ProviderSetupConfig,
607607
): Promise<boolean> {
608+
// A caller-supplied renderer (a headless test harness, or a live session's
609+
// renderer reused for a mid-session reconnect) is owned by that caller —
610+
// teardown here must not destroy it out from under them.
611+
const externalRenderer = config.createRenderer !== undefined
608612
const renderer = config.createRenderer
609613
? await config.createRenderer()
610614
: await createCliRenderer({
@@ -1075,10 +1079,12 @@ export async function runProviderSetup(
10751079
} catch {
10761080
// already unmounted
10771081
}
1078-
try {
1079-
renderer.destroy()
1080-
} catch {
1081-
// already destroyed
1082+
if (!externalRenderer) {
1083+
try {
1084+
renderer.destroy()
1085+
} catch {
1086+
// already destroyed
1087+
}
10821088
}
10831089
}
10841090

src/tui/provider-setup-submit.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,15 @@ export function buildProviderSubmitHandler(
2929
// tokens are already in the home-level auth store, and config load
3030
// projects that store into the provider catalog. Only the selection is
3131
// persisted here — the same two files /model writes when switching.
32+
//
33+
// Unlike a pasted key, this credential was just issued by the real
34+
// provider's own OAuth server completing a PKCE round-trip, so the
35+
// "unverified" concept the API-key path uses doesn't apply the same way
36+
// — there is no separate probe step to skip. What a completed login
37+
// does not confirm is that the resulting token actually carries API
38+
// scope (vs. e.g. a chat-only subscription), which can still surface as
39+
// a first-send auth error; tracked separately rather than faked here
40+
// with a flag this path has no real signal for.
3241
if (oauth !== undefined) {
3342
setPhase("saving");
3443
const base = existing ?? { providers: {} };

src/tui/runner.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -442,7 +442,7 @@ export async function runTUI(initialConfig: Config): Promise<number> {
442442
// connection test — warn now instead of a bare adapter error on first send.
443443
if (config.verified === false) {
444444
startupPluginNotices.push(
445-
`Provider "${config.providerName}" was saved without a passing connection test. If the first message fails with an auth error, run onboarding again to reconnect it.`,
445+
`We couldn't confirm your "${config.providerName}" key works. If your first message fails with an auth error, double-check the key.`,
446446
);
447447
}
448448
// Mutable list so trusting a project/path plugin can replace a metadata-only stub

0 commit comments

Comments
 (0)