Skip to content

Commit 71edfb0

Browse files
committed
fix(login): make sign-in cancellable and accept a provider's plain id
Four defects in the login flows, all reachable from a normal sign-in. The editor extension armed a cancel callback and never called it: the progress notification was not cancellable and no prompt received a cancellation token, so an OAuth flow waiting on a browser round trip had no way out at all. One login-wide cancellable notification now owns cancellation, and its token reaches every quick pick and input box. A second sign-in request started a rival flow behind a competing set of prompts; it now joins the one already running. A completed sign-in that failed only on the status refresh afterwards was reported as failed, sending the user back to a screen they had just finished. `--provider` matched a platform id or a display name, but a catalog provider's id carries an internal prefix and its label is a product name, so the id printed everywhere else - deepseek - matched neither. A cancelled OpenAI Codex sign-in returned before tearing its wait down, leaving both the callback timeout and the listening callback server to hold the host event loop for the remainder of two minutes.
1 parent c20f252 commit 71edfb0

8 files changed

Lines changed: 399 additions & 48 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pythoughts/pythinker-code": patch
3+
---
4+
5+
Accept a provider's plain id for `--provider` at login, so a catalog provider no longer has to be named by its full display name, and stop a cancelled OpenAI Codex sign-in from holding the process open for the rest of its two-minute callback timeout. In the editor extension, signing in now shows one cancellable progress notification, a repeated sign-in joins the one already running instead of opening a second set of prompts, and a completed sign-in is no longer reported as failed when the status refresh behind it fails.

apps/pythinker-code/test/cli/login.test.ts

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,11 @@ vi.mock('@pythoughts/pythinker-code-oauth', async () => {
8787
vi.mock('#/utils/open-url', () => ({ openUrl: vi.fn() }));
8888

8989
import { password, select } from '@clack/prompts';
90-
import { createPythinkerHarness } from '@pythoughts/pythinker-code-sdk';
90+
import {
91+
createPythinkerHarness,
92+
fetchCatalog,
93+
type Catalog,
94+
} from '@pythoughts/pythinker-code-sdk';
9195

9296
import { registerLoginCommand } from '#/cli/sub/login';
9397
import { openUrl } from '#/utils/open-url';
@@ -159,6 +163,26 @@ describe('pythinker login', () => {
159163
return stderrSpy.mock.calls.map((call: unknown[]) => String(call[0]));
160164
}
161165

166+
/** One connectable catalog provider, enough for `buildPlatformOptions` to list it. */
167+
function catalogWithDeepSeek(): Catalog {
168+
return {
169+
deepseek: {
170+
id: 'deepseek',
171+
name: 'DeepSeek',
172+
npm: '@ai-sdk/openai-compatible',
173+
api: 'https://api.example.com/v1',
174+
models: {
175+
'deepseek-chat': {
176+
id: 'deepseek-chat',
177+
name: 'DeepSeek Chat',
178+
tool_call: true,
179+
limit: { context: 128_000, output: 8_192 },
180+
},
181+
},
182+
},
183+
};
184+
}
185+
162186
it('registers a `login` subcommand with a --provider option on the program', () => {
163187
const program = new Command('pythinker');
164188
registerLoginCommand(program);
@@ -285,6 +309,44 @@ describe('pythinker login', () => {
285309
expect(exitSpy.mock.calls[0]?.[0]).toBe(0);
286310
});
287311

312+
it('--provider matches a catalog provider by its bare id', async () => {
313+
// A catalog provider's option value carries the internal `catalog:` prefix
314+
// and its label is a product name, so the id printed everywhere else —
315+
// `deepseek` — used to match neither and the login failed outright.
316+
mockStatus.mockResolvedValue({ providers: [] });
317+
mockGetConfig.mockResolvedValue({ providers: {}, models: {} });
318+
vi.mocked(fetchCatalog).mockResolvedValueOnce(catalogWithDeepSeek());
319+
vi.mocked(password).mockResolvedValue('sk-test-key');
320+
vi.mocked(select)
321+
.mockResolvedValueOnce('deepseek/deepseek-chat')
322+
.mockResolvedValueOnce('off');
323+
324+
await runLogin(['login', '--provider', 'DeepSeek API']);
325+
326+
expect(select).not.toHaveBeenCalledWith(
327+
expect.objectContaining({ message: 'Select a provider' }),
328+
);
329+
expect(mockSetConfig).toHaveBeenCalled();
330+
expect(exitSpy.mock.calls[0]?.[0]).toBe(0);
331+
332+
// The bare id resolves the same option the label does.
333+
vi.mocked(select).mockReset();
334+
mockSetConfig.mockClear();
335+
exitSpy.mockClear();
336+
vi.mocked(fetchCatalog).mockResolvedValueOnce(catalogWithDeepSeek());
337+
vi.mocked(select)
338+
.mockResolvedValueOnce('deepseek/deepseek-chat')
339+
.mockResolvedValueOnce('off');
340+
341+
await runLogin(['login', '--provider', 'deepseek']);
342+
343+
expect(select).not.toHaveBeenCalledWith(
344+
expect.objectContaining({ message: 'Select a provider' }),
345+
);
346+
expect(mockSetConfig).toHaveBeenCalled();
347+
expect(exitSpy.mock.calls[0]?.[0]).toBe(0);
348+
});
349+
288350
it('--provider with an unknown value exits non-zero, names the input, and never runs the picker', async () => {
289351
await runLogin(['login', '--provider', 'nope']);
290352

apps/vscode/src/auth/vscode-login-ui.ts

Lines changed: 54 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,14 @@ const BUILT_IN_CATALOG_JSON: string | undefined =
5151
const CATALOG_FETCH_TIMEOUT_MS = 10_000;
5252

5353
/** One quick pick for the effort level, from a list of supported levels. */
54-
async function promptEffortLevel(levels: readonly string[]): Promise<string | undefined> {
54+
async function promptEffortLevel(
55+
levels: readonly string[],
56+
token: vscode.CancellationToken,
57+
): Promise<string | undefined> {
5558
const selected = await vscode.window.showQuickPick(
5659
levels.map((level) => ({ label: level })),
5760
{ title: "Select effort level", placeHolder: "Select effort level", ignoreFocusOut: true },
61+
token,
5862
);
5963
return selected?.label;
6064
}
@@ -69,6 +73,10 @@ function createProgressHandle(title: string): LoginProgressSpinnerHandle {
6973
const finished = new Promise<void>((resolve) => {
7074
resolveFinished = resolve;
7175
});
76+
// Deliberately not cancellable: these track one step of a login, but the only
77+
// thing a cancel could do is abort the whole flow, and a Cancel button that
78+
// silently means more than its label is worse than none. The login-wide
79+
// progress notification the handler opens owns cancellation for every step.
7280
void vscode.window.withProgress(
7381
{ location: vscode.ProgressLocation.Notification, title, cancellable: false },
7482
() => finished,
@@ -92,8 +100,16 @@ function createProgressHandle(title: string): LoginProgressSpinnerHandle {
92100
* Build a `LoginUi` for the extension host, rendering every prompt with VS
93101
* Code's own widgets. The webview keeps receiving the OAuth device URL it
94102
* already renders (`Events.LoginUrl`), so the pending-login screen still works.
103+
*
104+
* `token` is the login-wide cancellation token owned by the caller. Every quick
105+
* pick and input box receives it, so cancelling closes whichever prompt is open
106+
* instead of leaving the user staring at a widget the flow has already given
107+
* up on; network waits are cancelled through `cancelInFlight`.
95108
*/
96-
export function createVscodeLoginUi(ctx: HandlerContext): LoginUi {
109+
export function createVscodeLoginUi(
110+
ctx: HandlerContext,
111+
token: vscode.CancellationToken,
112+
): LoginUi {
97113
let cancelInFlight: (() => void) | undefined;
98114

99115
function openBrowser(url: string): void {
@@ -168,38 +184,46 @@ export function createVscodeLoginUi(ctx: HandlerContext): LoginUi {
168184
description: option.description,
169185
value: option.value,
170186
}));
171-
const selected = await vscode.window.showQuickPick(items, {
172-
title: "Select a provider",
173-
placeHolder: "Select a provider",
174-
ignoreFocusOut: true,
175-
});
187+
const selected = await vscode.window.showQuickPick(
188+
items,
189+
{ title: "Select a provider", placeHolder: "Select a provider", ignoreFocusOut: true },
190+
token,
191+
);
176192
if (selected === undefined) return undefined;
177193
return { platformId: selected.value, catalog };
178194
},
179195
async promptApiKey(platformName, subtitleLines, promptOptions: ApiKeyPromptOptions = {}) {
180196
const emptyMessage = promptOptions.emptyMessage ?? "API key cannot be empty.";
181-
return vscode.window.showInputBox({
182-
title: promptOptions.title ?? `Enter API key for ${platformName}`,
183-
// An input box prompt is a single line, so newlines would collapse.
184-
prompt: subtitleLines?.join(" — "),
185-
password: promptOptions.secret !== false,
186-
ignoreFocusOut: true,
187-
validateInput: (input: string) => (input.length === 0 ? emptyMessage : undefined),
188-
});
197+
return vscode.window.showInputBox(
198+
{
199+
title: promptOptions.title ?? `Enter API key for ${platformName}`,
200+
// An input box prompt is a single line, so newlines would collapse.
201+
prompt: subtitleLines?.join(" — "),
202+
password: promptOptions.secret !== false,
203+
ignoreFocusOut: true,
204+
validateInput: (input: string) => (input.length === 0 ? emptyMessage : undefined),
205+
},
206+
token,
207+
);
189208
},
190209
async promptModelSelectionForOpenPlatform(models, platform) {
191210
const items = models.map((model) => ({
192211
label: model.displayName ?? model.id,
193212
model,
194213
}));
195-
const selected = await vscode.window.showQuickPick(items, {
196-
title: `Select a model for ${platform.name}`,
197-
placeHolder: "Select a model",
198-
ignoreFocusOut: true,
199-
});
214+
const selected = await vscode.window.showQuickPick(
215+
items,
216+
{
217+
title: `Select a model for ${platform.name}`,
218+
placeHolder: "Select a model",
219+
ignoreFocusOut: true,
220+
},
221+
token,
222+
);
200223
if (selected === undefined) return undefined;
201224
const effort = await promptEffortLevel(
202225
effortLevelsForModel(managedModelToAlias(platform.id, selected.model)),
226+
token,
203227
);
204228
if (effort === undefined) return undefined;
205229
return { model: selected.model, effort };
@@ -209,14 +233,19 @@ export function createVscodeLoginUi(ctx: HandlerContext): LoginUi {
209233
label: model.name ?? model.id,
210234
model,
211235
}));
212-
const selected = await vscode.window.showQuickPick(items, {
213-
title: `Select a model for ${providerId}`,
214-
placeHolder: "Select a model",
215-
ignoreFocusOut: true,
216-
});
236+
const selected = await vscode.window.showQuickPick(
237+
items,
238+
{
239+
title: `Select a model for ${providerId}`,
240+
placeHolder: "Select a model",
241+
ignoreFocusOut: true,
242+
},
243+
token,
244+
);
217245
if (selected === undefined) return undefined;
218246
const effort = await promptEffortLevel(
219247
effortLevelsForModel(catalogModelToAlias(providerId, selected.model)),
248+
token,
220249
);
221250
if (effort === undefined) return undefined;
222251
return { model: selected.model, effort };

apps/vscode/src/handlers/auth.handler.ts

Lines changed: 65 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,82 @@
11
import { runLogin } from "@pythoughts/pythinker-code-sdk";
2+
import * as vscode from "vscode";
23

34
import { Methods } from "../../shared/bridge";
45
import type { LoginResult } from "../../shared/legacy-sdk";
56
import type { LoginStatus } from "../../shared/types";
67
import { createVscodeLoginUi } from "../auth/vscode-login-ui";
78
import { updateLoginContext } from "../utils/context";
8-
import type { Handler } from "./types";
9+
import type { Handler, HandlerContext } from "./types";
10+
11+
/**
12+
* The login in flight, if any. Login owns modal UI — a second concurrent flow
13+
* would open a competing set of quick picks and race to write credentials — so
14+
* a repeated request joins the running one instead of starting a rival.
15+
*/
16+
let loginInFlight: Promise<LoginResult> | undefined;
17+
18+
async function runLoginOnce(ctx: HandlerContext): Promise<LoginResult> {
19+
// Owned here rather than inside the UI: the token has to exist before
20+
// `runLogin` starts, and cancelling has to reach whichever prompt is open,
21+
// not only the step that happens to hold a spinner.
22+
const cancellation = new vscode.CancellationTokenSource();
23+
const ui = createVscodeLoginUi(ctx, cancellation.token);
24+
try {
25+
// One login-wide progress notification, cancellable: the OAuth flows wait
26+
// minutes on a browser round trip with no per-step spinner of their own,
27+
// so this is the only surface that can offer a way out of them.
28+
const success = await vscode.window.withProgress(
29+
{
30+
location: vscode.ProgressLocation.Notification,
31+
title: "Signing in to Pythinker",
32+
cancellable: true,
33+
},
34+
async (_progress, token) => {
35+
token.onCancellationRequested(() => {
36+
ui.cancelInFlight?.();
37+
cancellation.cancel();
38+
});
39+
// `runLogin` resolves true only when credentials were written; a false
40+
// result is a user cancellation or a failure the flow already reported.
41+
return runLogin(ui);
42+
},
43+
);
44+
// Credentials are already on disk at this point, so a status refresh that
45+
// fails is a stale badge, not a failed login — reporting it as one would
46+
// send the webview back to the sign-in screen the user just completed.
47+
await updateLoginContext(ctx.harness).catch((statusError: unknown) => {
48+
ctx.logError("Unable to refresh login status after a successful login", statusError);
49+
});
50+
return { success };
51+
} catch (error) {
52+
ctx.logError("Pythinker login failed", error);
53+
await updateLoginContext(ctx.harness).catch((statusError: unknown) => {
54+
ctx.logError("Unable to refresh login status after a failed login", statusError);
55+
});
56+
return {
57+
success: false,
58+
error: error instanceof Error ? error.message : String(error),
59+
};
60+
} finally {
61+
cancellation.dispose();
62+
}
63+
}
964

1065
export const authHandlers: Record<string, Handler<any, any>> = {
1166
[Methods.CheckLoginStatus]: async (_, ctx): Promise<LoginStatus> => {
1267
return { loggedIn: await updateLoginContext(ctx.harness) };
1368
},
1469

1570
[Methods.Login]: async (_, ctx): Promise<LoginResult> => {
16-
try {
17-
// `runLogin` resolves true only when credentials were written; a false
18-
// result is a user cancellation or a failure the flow already reported.
19-
const success = await runLogin(createVscodeLoginUi(ctx));
20-
await updateLoginContext(ctx.harness);
21-
return { success };
22-
} catch (error) {
23-
ctx.logError("Pythinker login failed", error);
24-
await updateLoginContext(ctx.harness).catch((statusError: unknown) => {
25-
ctx.logError("Unable to refresh login status after a failed login", statusError);
26-
});
27-
return {
28-
success: false,
29-
error: error instanceof Error ? error.message : String(error),
30-
};
31-
}
71+
if (loginInFlight !== undefined) return loginInFlight;
72+
const pending = runLoginOnce(ctx);
73+
loginInFlight = pending;
74+
void pending.finally(() => {
75+
// Identity-checked so a slow flow that lost the slot cannot clear a newer
76+
// one and strand every later login on a cached result.
77+
if (loginInFlight === pending) loginInFlight = undefined;
78+
});
79+
return pending;
3280
},
3381

3482
[Methods.Logout]: async (_, ctx): Promise<LoginResult> => {

0 commit comments

Comments
 (0)