diff --git a/src/__tests__/cli.test.ts b/src/__tests__/cli.test.ts index 8bfd76741..a1dea4e51 100644 --- a/src/__tests__/cli.test.ts +++ b/src/__tests__/cli.test.ts @@ -667,6 +667,49 @@ describe('TUI auth task: region determines OAuth zone', () => { }), ); }); + + test('uses in-memory signupAuth tokens instead of disk or browser OAuth after TUI signup', async () => { + let storedCallback: (() => void) | null = null; + (mockStore.subscribe as any).mockImplementation((cb: () => void) => { + if (!storedCallback) storedCallback = cb; + return vi.fn(); + }); + + const cliPromise = runCLI(['--auth-onboarding', 'create-account']); + + await new Promise((r) => setTimeout(r, 50)); + mockStore.session = { + ...mockStore.session, + authOnboardingPath: 'create_account', + introConcluded: true, + region: 'us', + regionForced: false, + signupTokensObtained: true, + signupAuth: { + idToken: 'direct-id', + accessToken: 'direct-access', + refreshToken: 'direct-refresh', + zone: 'us', + userInfo: null, + dashboardUrl: null, + }, + signupAbandoned: false, + }; + (storedCallback as (() => void) | null)?.(); + + await cliPromise; + await waitFor(() => mockStore.setOAuthComplete.mock.calls.length > 0); + + expect(mockGetStoredToken).not.toHaveBeenCalled(); + expect(mockPerformAmplitudeAuth).not.toHaveBeenCalled(); + expect(mockStore.setOAuthComplete).toHaveBeenCalledWith( + expect.objectContaining({ + accessToken: 'direct-access', + idToken: 'direct-id', + cloudRegion: 'us', + }), + ); + }); }); // ── Feature discovery ────────────────────────────────────────────────────────── diff --git a/src/commands/default.ts b/src/commands/default.ts index 05c231281..45b4c8ed1 100644 --- a/src/commands/default.ts +++ b/src/commands/default.ts @@ -636,9 +636,7 @@ export const defaultCommand: CommandModule = { const { DEFAULT_AMPLITUDE_ZONE } = await import( '../lib/constants.js' ); - const { storeToken, getStoredToken } = await import( - '../utils/ampli-settings.js' - ); + const { storeToken } = await import('../utils/ampli-settings.js'); // Wait for the user to dismiss the welcome screen AND pick a // region before opening the OAuth URL. This ensures the logo @@ -719,14 +717,11 @@ export const defaultCommand: CommandModule = { // response — in which case we fall through to the existing OAuth flow // (TUI has a browser; this fallback is valid). // - // On signup success, the wrapper already fetched the real user - // profile (with provisioning retry) and persisted tokens to - // ~/.ampli.json. SigningUpScreen mirrors the wrapper-fetched - // userInfo onto `session.signupAuth.userInfo`, so reading it - // here lets us skip the redundant fetch + storeToken below. - // When the wrapper's fetch failed (provisioning lag exhausted - // retries), `signupAuth.userInfo` is null and we fall through - // to the probe path — same outcome as before. + // On signup success, SigningUpScreen captured fresh tokens in + // session. Use those in-memory tokens as the immediate handoff; + // disk persistence is a side effect, not coordination state. + // If wrapper-fetched userInfo is present too, skip the redundant + // fetch + storeToken below. let auth: Awaited< ReturnType > | null = null; @@ -743,32 +738,24 @@ export const defaultCommand: CommandModule = { '../utils/signup-or-auth.js' ); const s = tui.store.session; - if (s.signupTokensObtained) { + if (s.signupTokensObtained && s.signupAuth !== null) { // SigningUpScreen settled the ceremony successfully: - // `performSignupOrAuth` called `replaceStoredUser`, and // `setSignupAuth(non-null)` folded in - // `signupTokensObtained=true` atomically. Hydrate `auth` - // from disk here so `performAmplitudeAuth({ forceFresh })` - // below doesn't run on a fresh install dir and skip - // `~/.ampli.json` — that would open a spurious browser - // OAuth even though we already have valid tokens. + // `signupTokensObtained=true` atomically. signupTokensObtained = true; - const fromDisk = getStoredToken(undefined, zone); - if (fromDisk) { - auth = { - idToken: fromDisk.idToken, - accessToken: fromDisk.accessToken, - refreshToken: fromDisk.refreshToken, - zone, - }; - getUI().log.info( - 'Using signup tokens obtained during the signup ceremony.', - ); - } else { - getUI().log.warn( - 'Signup tokens were recorded but none found on disk; opening OAuth.', - ); - } + auth = { + idToken: s.signupAuth.idToken, + accessToken: s.signupAuth.accessToken, + refreshToken: s.signupAuth.refreshToken, + zone: s.signupAuth.zone, + }; + getUI().log.info( + 'Using signup tokens obtained during the signup ceremony.', + ); + } else if (s.signupTokensObtained) { + getUI().log.warn( + 'Signup tokens were recorded but signupAuth was missing; opening OAuth.', + ); } // Otherwise: ceremony abandoned (`signupAbandoned=true`) or // sign-in path. Auth gate would not have released without diff --git a/src/lib/api.ts b/src/lib/api.ts index 4808034b9..5804bfdfd 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -163,9 +163,15 @@ export class ApiError extends Error { * `projects` at this boundary so the rest of the wizard only sees the * user-facing terminology. */ +export interface FetchAmplitudeUserOptions { + timeoutMs?: number; + signal?: AbortSignal; +} + export async function fetchAmplitudeUser( idToken: string, zone: AmplitudeZone, + options: FetchAmplitudeUserOptions = {}, ): Promise { const { dataApiUrl } = AMPLITUDE_ZONE_SETTINGS[zone]; try { @@ -178,6 +184,8 @@ export async function fetchAmplitudeUser( 'Content-Type': 'application/json', 'User-Agent': WIZARD_USER_AGENT, }, + timeout: options.timeoutMs, + signal: options.signal, }, ); diff --git a/src/ui/tui/screens/SigningUpScreen.tsx b/src/ui/tui/screens/SigningUpScreen.tsx index 4d4f9b520..9ab47f57b 100644 --- a/src/ui/tui/screens/SigningUpScreen.tsx +++ b/src/ui/tui/screens/SigningUpScreen.tsx @@ -43,6 +43,16 @@ interface SigningUpScreenProps { store: WizardStore; } +function requiredFieldsSatisfied( + requiredFields: string[] | null, + fullName: string | null, +): boolean { + if (requiredFields === null) return false; + return requiredFields.every((field) => + field === 'full_name' ? fullName !== null : false, + ); +} + export const SigningUpScreen = ({ store }: SigningUpScreenProps) => { useWizardStore(store); @@ -54,7 +64,7 @@ export const SigningUpScreen = ({ store }: SigningUpScreenProps) => { // explicitly fixing. Without ToS, send email-only and let the server // route us to needs_information so the ToS screen renders next. const fullName = - session.tosAccepted === true ? (session.signupFullName ?? null) : null; + session.tosAccepted === true ? session.signupFullName ?? null : null; useAsyncEffect( async (signal) => { @@ -87,6 +97,21 @@ export const SigningUpScreen = ({ store }: SigningUpScreenProps) => { // error makes the contributor pick a behavior on purpose. switch (result.kind) { case 'success': + if ( + session.tosAccepted !== true || + !requiredFieldsSatisfied(session.signupRequiredFields, fullName) + ) { + log.warn( + 'signup: server returned success before required ceremony inputs were satisfied; abandoning', + { + hasRequiredFields: session.signupRequiredFields !== null, + tosAccepted: session.tosAccepted, + hasFullName: fullName !== null, + }, + ); + store.setSignupAbandoned(true); + return; + } // `setSignupAuth` folds in `signupTokensObtained=true` // atomically — the TUI auth-task gate releases on // `signupAuth` and reads `signupTokensObtained`; both must @@ -120,8 +145,9 @@ export const SigningUpScreen = ({ store }: SigningUpScreenProps) => { // `full_name` and the server is still asking for it), but // the cost of the guard is one branch and the failure mode // it prevents has zero in-band recovery. - const alreadySatisfied = result.requiredFields.every((field) => - field === 'full_name' ? fullName !== null : false, + const alreadySatisfied = requiredFieldsSatisfied( + result.requiredFields, + fullName, ); if (alreadySatisfied) { log.warn( diff --git a/src/ui/tui/screens/__tests__/SigningUpScreen.test.tsx b/src/ui/tui/screens/__tests__/SigningUpScreen.test.tsx new file mode 100644 index 000000000..1ca679d67 --- /dev/null +++ b/src/ui/tui/screens/__tests__/SigningUpScreen.test.tsx @@ -0,0 +1,51 @@ +import React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render } from 'ink-testing-library'; +import { SigningUpScreen } from '../SigningUpScreen.js'; +import { makeStoreForSnapshot } from '../../__tests__/snapshot-utils.js'; +import { waitForFrame } from '../../__tests__/ink-stdin.js'; + +const performSignupOrAuth = vi.hoisted(() => vi.fn()); + +vi.mock('../../../../utils/signup-or-auth.js', () => ({ + performSignupOrAuth, +})); + +describe('SigningUpScreen', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('abandons instead of accepting success from the email-only probe before ToS', async () => { + performSignupOrAuth.mockResolvedValue({ + kind: 'success', + idToken: 'direct-id', + accessToken: 'direct-access', + refreshToken: 'direct-refresh', + zone: 'us', + userInfo: null, + dashboardUrl: null, + }); + const store = makeStoreForSnapshot({ + region: 'us', + signupEmail: 'ada@example.com', + signupFullName: null, + signupRequiredFields: null, + tosAccepted: null, + }); + const setSignupAuthSpy = vi.spyOn(store, 'setSignupAuth'); + const setSignupAbandonedSpy = vi.spyOn(store, 'setSignupAbandoned'); + + const view = render(); + await waitForFrame(); + await waitForFrame(); + + expect(setSignupAuthSpy).not.toHaveBeenCalled(); + expect(setSignupAbandonedSpy).toHaveBeenCalledWith(true); + view.unmount(); + }); +}); diff --git a/src/utils/__tests__/direct-signup.test.ts b/src/utils/__tests__/direct-signup.test.ts index 527854a30..18b18fa6f 100644 --- a/src/utils/__tests__/direct-signup.test.ts +++ b/src/utils/__tests__/direct-signup.test.ts @@ -394,6 +394,31 @@ describe('performDirectSignup', () => { expect(result.kind).toBe('error'); }); + it('returns aborted when the provisioning POST is cancelled by the caller', async () => { + const controller = new AbortController(); + vi.spyOn(axios, 'post').mockImplementation(async () => { + controller.abort(); + const err = new Error('canceled') as Error & { code?: string }; + err.code = 'ERR_CANCELED'; + throw err; + }); + + try { + const result = await performDirectSignup({ + ...INPUT, + signal: controller.signal, + }); + + expect(result.kind).toBe('error'); + if (result.kind === 'error') { + expect(result.code).toBe('aborted'); + expect(result.message).toBe('aborted'); + } + } finally { + vi.restoreAllMocks(); + } + }); + it('routes EU requests to app.eu.amplitude.com', async () => { let observedUrl = ''; server.use( @@ -427,6 +452,37 @@ describe('performDirectSignup', () => { } }); + it('returns aborted when the token exchange POST is cancelled by the caller', async () => { + const controller = new AbortController(); + vi.spyOn(axios, 'post').mockImplementation(async (url: string) => { + if (url.includes('/t/agentic/signup/v1')) { + return { + status: 200, + data: { type: 'oauth', oauth: { code: 'auth-code-xyz' } }, + }; + } + controller.abort(); + const err = new Error('canceled') as Error & { code?: string }; + err.code = 'ERR_CANCELED'; + throw err; + }); + + try { + const result = await performDirectSignup({ + ...INPUT, + signal: controller.signal, + }); + + expect(result.kind).toBe('error'); + if (result.kind === 'error') { + expect(result.code).toBe('aborted'); + expect(result.message).toBe('aborted'); + } + } finally { + vi.restoreAllMocks(); + } + }); + it('returns error with parsed OAuth error on 400 token exchange response', async () => { server.use( http.post(PROVISIONING_URL, () => diff --git a/src/utils/__tests__/signup-or-auth.test.ts b/src/utils/__tests__/signup-or-auth.test.ts index 0ec9434c8..4c80191ef 100644 --- a/src/utils/__tests__/signup-or-auth.test.ts +++ b/src/utils/__tests__/signup-or-auth.test.ts @@ -452,6 +452,42 @@ describe('performSignupOrAuth', () => { } }); + it('falls back to pending sentinel when fetchAmplitudeUser hangs after direct-signup success', async () => { + vi.useFakeTimers(); + try { + const { performDirectSignup } = await import('../direct-signup.js'); + vi.mocked(performDirectSignup).mockResolvedValue({ + kind: 'success', + tokens: { + accessToken: 'direct-access', + idToken: 'direct-id', + refreshToken: 'direct-refresh', + expiresAt: new Date(Date.now() + 3_600_000).toISOString(), + zone: 'us', + }, + }); + const { fetchAmplitudeUser } = await import('../../lib/api.js'); + vi.mocked(fetchAmplitudeUser).mockReturnValue(new Promise(() => {})); + const { replaceStoredUser } = await import('../ampli-settings.js'); + + const pending = performSignupOrAuth({ + email: 'ada@example.com', + fullName: 'Ada Lovelace', + zone: 'us', + }); + await vi.runAllTimersAsync(); + const result = await pending; + + expect(replaceStoredUser).toHaveBeenCalledWith( + expect.objectContaining({ id: 'pending' }), + expect.anything(), + ); + expect(result).toMatchObject({ accessToken: 'direct-access' }); + } finally { + vi.useRealTimers(); + } + }); + it('emits agentic signup attempted with status=user_fetch_failed when fetch retries exhaust', async () => { vi.useFakeTimers(); try { @@ -491,6 +527,28 @@ describe('performSignupOrAuth', () => { } }); + it('does not emit signup_error telemetry when direct signup reports caller abort', async () => { + const { performDirectSignup } = await import('../direct-signup.js'); + vi.mocked(performDirectSignup).mockResolvedValue({ + kind: 'error', + code: 'aborted', + message: 'aborted', + }); + const { analytics } = await import('../analytics'); + + const result = await performSignupOrAuth({ + email: 'ada@example.com', + fullName: 'Ada Lovelace', + zone: 'us', + }); + + expect(result).toEqual({ kind: 'error', message: 'aborted' }); + expect(analytics.wizardCapture).not.toHaveBeenCalledWith( + AGENTIC_SIGNUP_ATTEMPTED_EVENT, + { status: 'signup_error', zone: 'us' }, + ); + }); + it('retries fetchAmplitudeUser when the new account has no env with an API key yet', async () => { vi.useFakeTimers(); try { diff --git a/src/utils/direct-signup.ts b/src/utils/direct-signup.ts index f0a4b19b3..b24def7bd 100644 --- a/src/utils/direct-signup.ts +++ b/src/utils/direct-signup.ts @@ -118,6 +118,20 @@ function provisioningUrl(zone: AmplitudeZone): string { return `${OUTBOUND_URLS.app[zone]}/t/agentic/signup/v1`; } +function isCallerAbort(error: unknown, signal?: AbortSignal): boolean { + if (signal?.aborted) return true; + if (axios.isCancel(error)) return true; + if (error instanceof Error) { + const maybeCode = (error as Error & { code?: string }).code; + return ( + error.name === 'AbortError' || + error.name === 'CanceledError' || + maybeCode === 'ERR_CANCELED' + ); + } + return false; +} + export interface DirectSignupInput { email: string; /** @@ -201,6 +215,9 @@ export async function performDirectSignup( signal: input.signal, }); } catch (e) { + if (isCallerAbort(e, input.signal)) { + return { kind: 'error', message: 'aborted', code: 'aborted' }; + } return { kind: 'error', message: e instanceof Error ? e.message : String(e), @@ -303,6 +320,9 @@ export async function performDirectSignup( }, ); } catch (e) { + if (isCallerAbort(e, input.signal)) { + return { kind: 'error', message: 'aborted', code: 'aborted' }; + } return { kind: 'error', message: `Token exchange failed: ${ diff --git a/src/utils/signup-or-auth.ts b/src/utils/signup-or-auth.ts index 0734d4929..4720f1d71 100644 --- a/src/utils/signup-or-auth.ts +++ b/src/utils/signup-or-auth.ts @@ -9,8 +9,84 @@ import { assertNever } from './assert-never.js'; const log = createLogger('signup-or-auth'); -// Retry delays for post-signup provisioning: worst-case total wait ~3.5s. +// Retry delays for post-signup provisioning. The delay budget is ~3.5s; +// each Data API call is separately bounded below so a hung user fetch +// can't strand the TUI auth gate forever after tokens were issued. const PROVISIONING_RETRY_DELAYS_MS = [500, 1000, 2000]; +const USER_FETCH_TIMEOUT_MS = 5_000; + +class UserFetchTimeoutError extends Error { + constructor(timeoutMs: number) { + super(`fetchAmplitudeUser timed out after ${timeoutMs}ms`); + this.name = 'TimeoutError'; + } +} + +function abortError(): Error { + const err = new Error('aborted'); + err.name = 'AbortError'; + return err; +} + +function isTimeoutOrAbort(error: unknown): boolean { + return ( + error instanceof UserFetchTimeoutError || + (error instanceof Error && + (error.name === 'AbortError' || error.name === 'CanceledError')) + ); +} + +async function abortableDelay( + delayMs: number, + signal?: AbortSignal, +): Promise { + if (signal?.aborted) throw abortError(); + let timer: ReturnType | undefined; + let onAbort: (() => void) | undefined; + try { + await new Promise((resolve, reject) => { + timer = setTimeout(resolve, delayMs); + if (signal) { + onAbort = () => reject(abortError()); + signal.addEventListener('abort', onAbort, { once: true }); + } + }); + } finally { + if (timer !== undefined) clearTimeout(timer); + if (signal && onAbort) signal.removeEventListener('abort', onAbort); + } +} + +async function fetchAmplitudeUserBounded( + idToken: string, + zone: AmplitudeZone, + signal?: AbortSignal, +): Promise { + if (signal?.aborted) throw abortError(); + let timer: ReturnType | undefined; + let onAbort: (() => void) | undefined; + try { + return await Promise.race([ + fetchAmplitudeUser(idToken, zone, { + timeoutMs: USER_FETCH_TIMEOUT_MS, + signal, + }), + new Promise((_, reject) => { + timer = setTimeout( + () => reject(new UserFetchTimeoutError(USER_FETCH_TIMEOUT_MS)), + USER_FETCH_TIMEOUT_MS, + ); + if (signal) { + onAbort = () => reject(abortError()); + signal.addEventListener('abort', onAbort, { once: true }); + } + }), + ]); + } finally { + if (timer !== undefined) clearTimeout(timer); + if (signal && onAbort) signal.removeEventListener('abort', onAbort); + } +} function hasEnvWithApiKey(userInfo: AmplitudeUserInfo): boolean { return userInfo.orgs.some((org) => @@ -44,14 +120,18 @@ type FetchUserResult = async function fetchUserWithProvisioningRetry( idToken: string, zone: AmplitudeZone, + signal?: AbortSignal, ): Promise { let userInfo: AmplitudeUserInfo | null = null; let lastError: unknown = null; let retryCount = 0; try { - userInfo = await fetchAmplitudeUser(idToken, zone); + userInfo = await fetchAmplitudeUserBounded(idToken, zone, signal); } catch (err) { lastError = err; + if (isTimeoutOrAbort(err)) { + return { ok: false, retryCount, error: lastError }; + } } for (const delayMs of PROVISIONING_RETRY_DELAYS_MS) { if (userInfo && hasEnvWithApiKey(userInfo)) { @@ -61,16 +141,23 @@ async function fetchUserWithProvisioningRetry( delayMs, threw: lastError !== null, }); - await new Promise((resolve) => setTimeout(resolve, delayMs)); + try { + await abortableDelay(delayMs, signal); + } catch (err) { + return { ok: false, retryCount, error: err }; + } retryCount += 1; try { - userInfo = await fetchAmplitudeUser(idToken, zone); + userInfo = await fetchAmplitudeUserBounded(idToken, zone, signal); lastError = null; } catch (err) { // Keep any prior successful userInfo — losing it here would make us // fall back to the pending sentinel when we already have real user // data from an earlier attempt that just didn't yet have an env. lastError = err; + if (isTimeoutOrAbort(err)) { + return { ok: false, retryCount, error: lastError }; + } } } if (userInfo) { @@ -257,6 +344,9 @@ export async function performSignupOrAuth( message: result.message, code: result.code, }); + if (result.code === 'aborted') { + return { kind: 'error', message: result.message }; + } // The schema's `.refine()` on `required` rejects shapes the wizard // can't act on, and `direct-signup.ts` surfaces that with // `code: 'unsupported_required_shape'`. Emit a distinct telemetry @@ -294,6 +384,7 @@ export async function performSignupOrAuth( const fetchResult = await fetchUserWithProvisioningRetry( tokens.idToken, input.zone, + input.signal, ); if (fetchResult.ok) { userInfo = fetchResult.userInfo;