From 2298dd3361030695d2fa8bc2bd009594987f3a16 Mon Sep 17 00:00:00 2001 From: Gray Gilmore Date: Tue, 15 Sep 2026 13:27:20 -0700 Subject: [PATCH 1/7] Handle Trust Battery challenges for theme mutations Core runs a Trust Battery session decision on every theme mutation. Admin Web can present the resulting identity challenge, but the CLI cannot, so Core currently suppresses challenges for CLI callers. That leaves the CLI as a way around the check, and Core will start surfacing challenges to CLI versions that can handle them. Theme Admin API requests now recognise a CHALLENGE_REQUIRED GraphQL error, open the challenge URL in the browser, wait for the user to press Enter once they have completed it, and retry the request once. A second challenge on the retry aborts with the URL instead of looping. Non-interactive runs (CI or no TTY) abort immediately with the URL and next steps. Concurrent requests, such as the parallel upload batches of `theme push`, share one prompt so the browser opens once and every waiting batch retries after the same verification. A request that starts while a challenge is still pending waits for it first, so it is sent once instead of being challenged and retried too. The prompt accepts Enter or Space only. `theme dev` keeps stdin in raw mode and binds single-letter shortcuts on the same stream; the dedicated wait ignores other keys and leaves raw mode as it found it, so answering the prompt never fires a shortcut. Only challenge URLs on the Admin host are honoured; anything else falls through to normal error handling. The handling is intentionally scoped to the theme API layer; app commands are unchanged. --- .changeset/theme-trust-challenge.md | 5 + .../node/themes/trust-challenge.test.ts | 285 ++++++++++++++++++ .../private/node/themes/trust-challenge.ts | 160 ++++++++++ .../private/node/ui/wait-for-enter.test.ts | 161 ++++++++++ .../src/private/node/ui/wait-for-enter.ts | 49 +++ .../src/public/node/themes/api.test.ts | 95 ++++++ .../cli-kit/src/public/node/themes/api.ts | 3 +- 7 files changed, 757 insertions(+), 1 deletion(-) create mode 100644 .changeset/theme-trust-challenge.md create mode 100644 packages/cli-kit/src/private/node/themes/trust-challenge.test.ts create mode 100644 packages/cli-kit/src/private/node/themes/trust-challenge.ts create mode 100644 packages/cli-kit/src/private/node/ui/wait-for-enter.test.ts create mode 100644 packages/cli-kit/src/private/node/ui/wait-for-enter.ts diff --git a/.changeset/theme-trust-challenge.md b/.changeset/theme-trust-challenge.md new file mode 100644 index 00000000000..be0feb511cb --- /dev/null +++ b/.changeset/theme-trust-challenge.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': minor +--- + +Handle Trust Battery identity challenges for theme commands. When Shopify asks to verify your identity before a theme change, the CLI opens the verification page in your browser and retries once you complete it. diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts new file mode 100644 index 00000000000..bcc0d4271d1 --- /dev/null +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts @@ -0,0 +1,285 @@ +import {trustChallengeUrl, withTrustChallengeRetry} from './trust-challenge.js' +import {GraphQLClientError} from '../api/headers.js' +import {waitForEnter} from '../ui/wait-for-enter.js' +import {recordEvent} from '../../../public/node/analytics.js' +import {adminFqdn} from '../../../public/node/context/fqdn.js' +import {AbortError, AbortSilentError} from '../../../public/node/error.js' +import {isCI, openURL} from '../../../public/node/system.js' +import {isTTY, renderInfo, renderSuccess} from '../../../public/node/ui.js' +import {beforeEach, describe, expect, test, vi} from 'vitest' +import {ClientError} from 'graphql-request' + +vi.mock('../ui/wait-for-enter.js') +vi.mock('../../../public/node/analytics.js') +vi.mock('../../../public/node/context/fqdn.js') +vi.mock('../../../public/node/output.js') +vi.mock('../../../public/node/system.js') +vi.mock('../../../public/node/ui.js') + +const challengeUrl = 'https://admin.shopify.com/challenges/user_verification?token=abc' + +beforeEach(() => { + vi.mocked(adminFqdn).mockResolvedValue('admin.shopify.com') + vi.mocked(isCI).mockReturnValue(false) + vi.mocked(isTTY).mockReturnValue(true) + vi.mocked(openURL).mockResolvedValue(true) + vi.mocked(waitForEnter).mockResolvedValue(undefined) +}) + +describe('trustChallengeUrl', () => { + test('returns the redirect URL from a CHALLENGE_REQUIRED graphql-request ClientError', async () => { + const error = challengeClientError(challengeUrl) + + await expect(trustChallengeUrl(error)).resolves.toBe(challengeUrl) + }) + + test('returns the redirect URL from a CHALLENGE_REQUIRED cli-kit GraphQLClientError', async () => { + const error = new GraphQLClientError('Challenge Required', 200, [ + {message: 'Challenge Required', extensions: {code: 'CHALLENGE_REQUIRED', redirect_to: challengeUrl}}, + ]) + + await expect(trustChallengeUrl(error)).resolves.toBe(challengeUrl) + }) + + test('returns undefined for a GraphQL error with a different code', async () => { + const error = new ClientError( + {status: 200, errors: [{message: 'Access denied', extensions: {code: 'ACCESS_DENIED'}} as any]}, + {query: ''}, + ) + + await expect(trustChallengeUrl(error)).resolves.toBeUndefined() + }) + + test('returns undefined when the redirect URL is not on the Admin host', async () => { + const error = challengeClientError('https://evil.example.com/challenges/user_verification?token=abc') + + await expect(trustChallengeUrl(error)).resolves.toBeUndefined() + }) + + test('returns undefined when the redirect URL is not https', async () => { + const error = challengeClientError('http://admin.shopify.com/challenges/user_verification?token=abc') + + await expect(trustChallengeUrl(error)).resolves.toBeUndefined() + }) + + test('returns undefined when the redirect URL is missing', async () => { + const error = new ClientError( + {status: 200, errors: [{message: 'Challenge Required', extensions: {code: 'CHALLENGE_REQUIRED'}} as any]}, + {query: ''}, + ) + + await expect(trustChallengeUrl(error)).resolves.toBeUndefined() + }) + + test('returns undefined for an error that is not a GraphQL client error', async () => { + await expect(trustChallengeUrl(new Error('boom'))).resolves.toBeUndefined() + }) + + test('accepts the local Admin host when the CLI targets a local environment', async () => { + vi.mocked(adminFqdn).mockResolvedValue('admin.shop.dev') + const localChallengeUrl = 'https://admin.shop.dev/challenges/user_verification?token=abc' + + await expect(trustChallengeUrl(challengeClientError(localChallengeUrl))).resolves.toBe(localChallengeUrl) + }) +}) + +describe('withTrustChallengeRetry', () => { + test('returns the result of a request that succeeds without a challenge', async () => { + const request = vi.fn().mockResolvedValue('ok') + + await expect(withTrustChallengeRetry(request)).resolves.toBe('ok') + + expect(request).toHaveBeenCalledTimes(1) + expect(openURL).not.toHaveBeenCalled() + expect(waitForEnter).not.toHaveBeenCalled() + }) + + test('rethrows an error that is not a trust challenge without prompting', async () => { + const error = new Error('boom') + const request = vi.fn().mockRejectedValue(error) + + await expect(withTrustChallengeRetry(request)).rejects.toBe(error) + + expect(request).toHaveBeenCalledTimes(1) + expect(openURL).not.toHaveBeenCalled() + }) + + test('opens the challenge URL, waits for Enter, then retries the request once', async () => { + const request = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)).mockResolvedValueOnce('retried') + + await expect(withTrustChallengeRetry(request)).resolves.toBe('retried') + + expect(request).toHaveBeenCalledTimes(2) + expect(openURL).toHaveBeenCalledWith(challengeUrl) + expect(waitForEnter).toHaveBeenCalledTimes(1) + expect(renderInfo).toHaveBeenCalledWith( + expect.objectContaining({ + headline: 'Verify your identity to continue.', + nextSteps: expect.arrayContaining([expect.arrayContaining([{userInput: 'Enter'}])]), + }), + ) + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:required') + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:browser-opened') + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:acknowledged') + expect(renderSuccess).toHaveBeenCalledWith({headline: 'Continuing with your theme changes.'}) + }) + + test('renders a link to the challenge URL when the browser cannot be opened', async () => { + vi.mocked(openURL).mockResolvedValue(false) + const request = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)).mockResolvedValueOnce('retried') + + await withTrustChallengeRetry(request) + + expect(renderInfo).toHaveBeenCalledWith( + expect.objectContaining({link: {label: 'Open the verification page', url: challengeUrl}}), + ) + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:browser-not-opened') + }) + + test('aborts with the challenge URL when the retried request is challenged again', async () => { + const request = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + + const promise = withTrustChallengeRetry(request) + + await expect(promise).rejects.toBeInstanceOf(AbortError) + await expect(promise).rejects.toMatchObject({ + nextSteps: [expect.arrayContaining([{link: {url: challengeUrl}}])], + }) + expect(request).toHaveBeenCalledTimes(2) + expect(waitForEnter).toHaveBeenCalledTimes(1) + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:retry-challenged') + }) + + test('rethrows a non-challenge error from the retried request unchanged', async () => { + const retryError = new Error('boom') + const request = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)).mockRejectedValueOnce(retryError) + + await expect(withTrustChallengeRetry(request)).rejects.toBe(retryError) + + expect(request).toHaveBeenCalledTimes(2) + }) + + test('aborts with the challenge URL in the next steps when running in CI', async () => { + vi.mocked(isCI).mockReturnValue(true) + const request = vi.fn().mockRejectedValue(challengeClientError(challengeUrl)) + + const promise = withTrustChallengeRetry(request) + + await expect(promise).rejects.toBeInstanceOf(AbortError) + await expect(promise).rejects.toMatchObject({ + nextSteps: [expect.arrayContaining([{link: {url: challengeUrl}}])], + }) + expect(request).toHaveBeenCalledTimes(1) + expect(openURL).not.toHaveBeenCalled() + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:non-interactive') + }) + + test('aborts instead of prompting when stdin is not a TTY', async () => { + vi.mocked(isTTY).mockReturnValue(false) + const request = vi.fn().mockRejectedValue(challengeClientError(challengeUrl)) + + await expect(withTrustChallengeRetry(request)).rejects.toBeInstanceOf(AbortError) + + expect(waitForEnter).not.toHaveBeenCalled() + }) + + test('shares a single prompt across concurrent requests that are challenged together', async () => { + const enter = deferredEnter() + const firstRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('first') + const secondRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('second') + + const results = Promise.all([withTrustChallengeRetry(firstRequest), withTrustChallengeRetry(secondRequest)]) + await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(1)) + enter.press() + + await expect(results).resolves.toEqual(['first', 'second']) + expect(openURL).toHaveBeenCalledTimes(1) + expect(renderSuccess).toHaveBeenCalledTimes(1) + expect(firstRequest).toHaveBeenCalledTimes(2) + expect(secondRequest).toHaveBeenCalledTimes(2) + }) + + test('holds a request that starts while a challenge is pending until Enter is pressed, then sends it once', async () => { + const enter = deferredEnter() + const challengedRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('challenged') + const laterRequest = vi.fn().mockResolvedValue('later') + + const challengedResult = withTrustChallengeRetry(challengedRequest) + await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(1)) + + const laterResult = withTrustChallengeRetry(laterRequest) + await Promise.resolve() + expect(laterRequest).not.toHaveBeenCalled() + + enter.press() + + await expect(Promise.all([challengedResult, laterResult])).resolves.toEqual(['challenged', 'later']) + expect(laterRequest).toHaveBeenCalledTimes(1) + expect(openURL).toHaveBeenCalledTimes(1) + }) + + test('rejects requests waiting on a pending challenge when the prompt is cancelled with Ctrl+C', async () => { + const enter = deferredEnter() + const challengedRequest = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)) + const laterRequest = vi.fn().mockResolvedValue('later') + + const challengedResult = withTrustChallengeRetry(challengedRequest) + await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(1)) + const laterResult = withTrustChallengeRetry(laterRequest) + + enter.cancel() + + await expect(challengedResult).rejects.toBeInstanceOf(AbortSilentError) + await expect(laterResult).rejects.toBeInstanceOf(AbortSilentError) + expect(laterRequest).not.toHaveBeenCalled() + expect(renderSuccess).not.toHaveBeenCalled() + }) + + test('passes the provided stdin to the Enter prompt', async () => { + const stdin = {} as typeof process.stdin + const request = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)).mockResolvedValueOnce('retried') + + await withTrustChallengeRetry(request, {stdin}) + + expect(waitForEnter).toHaveBeenCalledWith(stdin) + }) +}) + +function deferredEnter(): {press: () => void; cancel: () => void} { + let press!: () => void + let cancel!: () => void + vi.mocked(waitForEnter).mockReturnValue( + new Promise((resolve, reject) => { + press = resolve + cancel = () => reject(new AbortSilentError()) + }), + ) + return {press: () => press(), cancel: () => cancel()} +} + +function challengeClientError(redirectTo: string): ClientError { + return new ClientError( + { + status: 200, + errors: [ + { + message: 'Challenge Required', + extensions: {code: 'CHALLENGE_REQUIRED', redirect_to: redirectTo}, + } as any, + ], + }, + {query: ''}, + ) +} diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.ts new file mode 100644 index 00000000000..86eb012aa08 --- /dev/null +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.ts @@ -0,0 +1,160 @@ +import {GraphQLClientError} from '../api/headers.js' +import {waitForEnter} from '../ui/wait-for-enter.js' +import {recordEvent} from '../../../public/node/analytics.js' +import {adminFqdn} from '../../../public/node/context/fqdn.js' +import {AbortError} from '../../../public/node/error.js' +import {outputDebug} from '../../../public/node/output.js' +import {isCI, openURL} from '../../../public/node/system.js' +import {isTTY, renderInfo, renderSuccess} from '../../../public/node/ui.js' +import {ClientError} from 'graphql-request' + +export const TRUST_CHALLENGE_ERROR_CODE = 'CHALLENGE_REQUIRED' + +interface GraphQLErrorWithExtensions { + extensions?: { + code?: unknown + redirect_to?: unknown + } +} + +export interface TrustChallengeOptions { + stdin?: typeof process.stdin +} + +let pendingChallenge: Promise | undefined + +/** + * Runs a theme Admin API request and, when Core answers with a Trust Battery + * challenge, waits for the user to complete it in the browser before retrying once. + * + * A request that starts while another request's challenge is still being completed + * waits for that challenge first, so it is sent once instead of being challenged + * itself and then retried. + * + * @param request - The request to run and possibly retry. + * @param options - Overrides for the prompt, mainly for tests. + * @returns The result of the request. + */ +export async function withTrustChallengeRetry( + request: () => Promise, + options: TrustChallengeOptions = {}, +): Promise { + if (pendingChallenge) await pendingChallenge + + try { + return await request() + } catch (error) { + const challengeUrl = await trustChallengeUrl(error) + if (!challengeUrl) throw error + + await completeTrustChallenge(challengeUrl, options) + } + + try { + return await request() + } catch (error) { + const challengeUrl = await trustChallengeUrl(error) + if (!challengeUrl) throw error + + recordEvent('theme-api:trust-challenge:retry-challenged') + throw new AbortError( + 'Shopify still needs to verify your identity before it can apply this theme change.', + undefined, + [['Complete the verification at', {link: {url: challengeUrl}}, 'then run the command again.']], + ) + } +} + +/** + * Extracts the challenge URL from a Trust Battery `CHALLENGE_REQUIRED` GraphQL error. + * + * Only URLs on the Admin host are trusted. Anything else is not treated as a + * challenge and the original error surfaces through normal error handling. + * + * @param error - The error thrown by an Admin API request. + * @returns The challenge URL, or undefined when the error is not a trust challenge. + */ +export async function trustChallengeUrl(error: unknown): Promise { + const challengeError = graphQLErrors(error).find((graphQLError) => { + return graphQLError?.extensions?.code === TRUST_CHALLENGE_ERROR_CODE + }) + const redirectTo = challengeError?.extensions?.redirect_to + if (typeof redirectTo !== 'string') return undefined + + if (!(await isAdminUrl(redirectTo))) { + outputDebug(`Ignoring trust challenge with an unexpected URL host: ${redirectTo}`) + return undefined + } + + return redirectTo +} + +function graphQLErrors(error: unknown): GraphQLErrorWithExtensions[] { + if (error instanceof ClientError) { + return Array.isArray(error.response.errors) ? error.response.errors : [] + } + if (error instanceof GraphQLClientError) { + return Array.isArray(error.errors) ? error.errors : [] + } + return [] +} + +async function isAdminUrl(url: string): Promise { + try { + const parsedUrl = new URL(url) + return parsedUrl.protocol === 'https:' && parsedUrl.hostname === (await adminFqdn()) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + return false + } +} + +/** + * Concurrent theme requests (for example the parallel batches of `theme push`) + * all receive the same challenge. Share one prompt so the browser opens once and + * every waiting request retries after the same verification. + */ +async function completeTrustChallenge(challengeUrl: string, options: TrustChallengeOptions): Promise { + pendingChallenge ??= promptForTrustChallenge(challengeUrl, options).finally(() => { + pendingChallenge = undefined + }) + return pendingChallenge +} + +async function promptForTrustChallenge(challengeUrl: string, options: TrustChallengeOptions): Promise { + recordEvent('theme-api:trust-challenge:required') + + if (isCI() || !isTTY()) { + recordEvent('theme-api:trust-challenge:non-interactive') + throw new AbortError('Shopify needs to verify your identity before it can apply this theme change.', undefined, [ + [ + 'Open', + {link: {url: challengeUrl}}, + "in a browser where you're logged in to Shopify, complete the verification, then run the command again.", + ], + ]) + } + + const opened = await openURL(challengeUrl) + recordEvent(opened ? 'theme-api:trust-challenge:browser-opened' : 'theme-api:trust-challenge:browser-not-opened') + + // Everything the user needs is inside the banner: long-running commands such as + // `theme push` redraw an Ink progress bar that erases any plain line written after it. + renderInfo({ + headline: 'Verify your identity to continue.', + body: opened + ? 'A verification page opened in your browser.' + : 'Shopify needs to verify your identity before it can apply your theme changes.', + link: opened ? undefined : {label: 'Open the verification page', url: challengeUrl}, + nextSteps: [ + ['Complete the verification in your browser.'], + ['Press', {userInput: 'Enter'}, 'here once you have completed it.'], + ], + orderedNextSteps: true, + }) + + await waitForEnter(options.stdin ?? process.stdin) + + recordEvent('theme-api:trust-challenge:acknowledged') + renderSuccess({headline: 'Continuing with your theme changes.'}) +} diff --git a/packages/cli-kit/src/private/node/ui/wait-for-enter.test.ts b/packages/cli-kit/src/private/node/ui/wait-for-enter.test.ts new file mode 100644 index 00000000000..1c80f4ae7fe --- /dev/null +++ b/packages/cli-kit/src/private/node/ui/wait-for-enter.test.ts @@ -0,0 +1,161 @@ +import {waitForEnter} from './wait-for-enter.js' +import {AbortSilentError} from '../../../public/node/error.js' +import {describe, expect, test, vi} from 'vitest' +import {EventEmitter} from 'events' + +class FakeStdin extends EventEmitter { + isRaw: boolean + setRawMode = vi.fn((mode: boolean) => { + this.isRaw = mode + return this + }) + + ref = vi.fn(() => this) + unref = vi.fn(() => this) + + constructor({isRaw}: {isRaw: boolean}) { + super() + this.isRaw = isRaw + } + + press(key: string): void { + this.emit('data', Buffer.from(key)) + } + + pressEncoded(key: string): void { + this.emit('data', key) + } + + asStdin(): typeof process.stdin { + return this as unknown as typeof process.stdin + } +} + +describe('waitForEnter', () => { + test('resolves when Enter is pressed as a carriage return', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = waitForEnter(stdin.asStdin()) + + stdin.press('\r') + + await expect(promise).resolves.toBeUndefined() + }) + + test('resolves when Enter is pressed as a line feed', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = waitForEnter(stdin.asStdin()) + + stdin.press('\n') + + await expect(promise).resolves.toBeUndefined() + }) + + test('resolves when Space is pressed', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = waitForEnter(stdin.asStdin()) + + stdin.press(' ') + + await expect(promise).resolves.toBeUndefined() + }) + + test('ignores other keys until Enter is pressed', async () => { + const stdin = new FakeStdin({isRaw: false}) + const settled = vi.fn() + const promise = waitForEnter(stdin.asStdin()).then(settled) + + stdin.press('t') + stdin.press('p') + await Promise.resolve() + expect(settled).not.toHaveBeenCalled() + + stdin.press('\r') + + await expect(promise).resolves.toBeUndefined() + }) + + test('rejects with AbortSilentError when Ctrl+C is pressed', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = waitForEnter(stdin.asStdin()) + + stdin.press('\u0003') + + await expect(promise).rejects.toBeInstanceOf(AbortSilentError) + }) + + test('resolves when Enter arrives as a utf8 string chunk because another renderer set the stdin encoding', async () => { + const stdin = new FakeStdin({isRaw: true}) + const promise = waitForEnter(stdin.asStdin()) + + stdin.pressEncoded('\r') + + await expect(promise).resolves.toBeUndefined() + }) + + test('ignores other keys that arrive as utf8 string chunks', async () => { + const stdin = new FakeStdin({isRaw: true}) + const settled = vi.fn() + const promise = waitForEnter(stdin.asStdin()).then(settled) + + stdin.pressEncoded('t') + await Promise.resolve() + expect(settled).not.toHaveBeenCalled() + + stdin.pressEncoded('\n') + + await expect(promise).resolves.toBeUndefined() + }) + + test('rejects with AbortSilentError when Ctrl+C arrives as a utf8 string chunk', async () => { + const stdin = new FakeStdin({isRaw: true}) + const promise = waitForEnter(stdin.asStdin()) + + stdin.pressEncoded('\u0003') + + await expect(promise).rejects.toBeInstanceOf(AbortSilentError) + }) + + test('resolves when Enter is part of a multi-character chunk', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = waitForEnter(stdin.asStdin()) + + stdin.press('ab\r') + + await expect(promise).resolves.toBeUndefined() + }) + + test('turns raw mode on while waiting and back off afterwards when stdin was not raw', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = waitForEnter(stdin.asStdin()) + + expect(stdin.setRawMode).toHaveBeenCalledWith(true) + stdin.press('\r') + await promise + + expect(stdin.setRawMode).toHaveBeenLastCalledWith(false) + }) + + test('leaves raw mode alone when stdin was already raw', async () => { + const stdin = new FakeStdin({isRaw: true}) + const promise = waitForEnter(stdin.asStdin()) + + stdin.press('\r') + await promise + + expect(stdin.setRawMode).not.toHaveBeenCalled() + expect(stdin.isRaw).toBe(true) + }) + + test('stops listening to stdin and unrefs it once settled', async () => { + const stdin = new FakeStdin({isRaw: true}) + const promise = waitForEnter(stdin.asStdin()) + + expect(stdin.ref).toHaveBeenCalledTimes(1) + expect(stdin.listenerCount('data')).toBe(1) + stdin.press('\r') + await promise + + expect(stdin.listenerCount('data')).toBe(0) + expect(stdin.unref).toHaveBeenCalledTimes(1) + }) +}) diff --git a/packages/cli-kit/src/private/node/ui/wait-for-enter.ts b/packages/cli-kit/src/private/node/ui/wait-for-enter.ts new file mode 100644 index 00000000000..c1afa6dfc1f --- /dev/null +++ b/packages/cli-kit/src/private/node/ui/wait-for-enter.ts @@ -0,0 +1,49 @@ +import {AbortSilentError} from '../../../public/node/error.js' +import {runWithTimer} from '../../../public/node/metadata.js' + +const CTRL_C = '\u0003' +const ACCEPTED_KEYS = new Set(['\r', '\n', ' ']) + +/** + * Waits for Enter (or Space) on stdin and ignores every other key. Ctrl+C rejects + * with AbortSilentError so the process exits the same way it would from any prompt. + * + * Raw mode is left the way it was found. Long-running commands such as `theme dev` + * keep stdin raw for their own single-key shortcuts, and the accepted keys are ones + * those commands do not bind, so pressing Enter here never triggers a shortcut. + * + * Chunks are compared as text rather than bytes: an active Ink renderer (for example + * the `theme push` progress bar) calls `stdin.setEncoding('utf8')`, after which stdin + * emits strings instead of Buffers. + * + * @param stdin - The stream to read from; defaults to process.stdin. + * @returns A promise that resolves when Enter or Space is pressed. + */ +export function waitForEnter(stdin: typeof process.stdin = process.stdin): Promise { + return runWithTimer('cmd_all_timing_prompts_ms')(() => { + return new Promise((resolve, reject) => { + const stdinWasRaw = stdin.isRaw === true + + const cleanup = () => { + stdin.off('data', onData) + if (!stdinWasRaw) stdin.setRawMode(false) + stdin.unref() + } + + const onData = (chunk: Buffer | string) => { + const text = typeof chunk === 'string' ? chunk : chunk.toString('utf8') + if (text.includes(CTRL_C)) { + cleanup() + reject(new AbortSilentError()) + } else if ([...text].some((character) => ACCEPTED_KEYS.has(character))) { + cleanup() + resolve() + } + } + + if (!stdinWasRaw) stdin.setRawMode(true) + stdin.ref() + stdin.on('data', onData) + }) + }) +} diff --git a/packages/cli-kit/src/public/node/themes/api.test.ts b/packages/cli-kit/src/public/node/themes/api.test.ts index abf10d832f0..34b9b43f165 100644 --- a/packages/cli-kit/src/public/node/themes/api.test.ts +++ b/packages/cli-kit/src/public/node/themes/api.test.ts @@ -31,11 +31,18 @@ import {MetafieldDefinitionsByOwnerType} from '../../../cli/api/graphql/admin/ge import {errorHandler} from '../../../private/node/api/graphql.js' import {adminRequestDoc, supportedApiVersions} from '../api/admin.js' import {AbortError} from '../error.js' +import {adminFqdn} from '../context/fqdn.js' +import {openURL} from '../system.js' +import {isTTY} from '../ui.js' +import {waitForEnter} from '../../../private/node/ui/wait-for-enter.js' import {test, vi, expect, describe, beforeEach} from 'vitest' import {ClientError} from 'graphql-request' vi.mock('../api/admin.js') +vi.mock('../context/fqdn.js') +vi.mock('../ui.js') +vi.mock('../../../private/node/ui/wait-for-enter.js') vi.mock('@shopify/cli-kit/node/system') vi.stubGlobal('fetch', vi.fn()) @@ -562,6 +569,79 @@ describe('themePublish', () => { }) }) +describe('trust challenges', () => { + const challengeUrl = 'https://admin.shopify.com/challenges/user_verification?token=abc' + + beforeEach(() => { + vi.mocked(adminFqdn).mockResolvedValue('admin.shopify.com') + vi.mocked(isTTY).mockReturnValue(true) + vi.mocked(openURL).mockResolvedValue(true) + vi.mocked(waitForEnter).mockResolvedValue(undefined) + }) + + test('themePublish opens the challenge in the browser and retries after the user completes it', async () => { + // Given + const id = 123 + vi.mocked(adminRequestDoc) + .mockRejectedValueOnce(trustChallengeError(challengeUrl)) + .mockResolvedValueOnce({ + themePublish: { + theme: {id: `gid://shopify/OnlineStoreTheme/${id}`, name: 'live theme', role: 'live'}, + userErrors: [], + }, + }) + + // When + const theme = await themePublish(id, session) + + // Then + expect(openURL).toHaveBeenCalledWith(challengeUrl) + expect(waitForEnter).toHaveBeenCalledTimes(1) + expect(adminRequestDoc).toHaveBeenCalledTimes(2) + expect(theme!.id).toEqual(id) + }) + + test('bulkUploadThemeAssets retries the challenged batch after the user completes the challenge', async () => { + // Given + vi.mocked(adminRequestDoc) + .mockRejectedValueOnce(trustChallengeError(challengeUrl)) + .mockResolvedValueOnce({ + themeFilesUpsert: { + upsertedThemeFiles: [{filename: 'snippets/product-variant-picker.liquid'}], + userErrors: [], + }, + }) + + // When + const results = await bulkUploadThemeAssets( + 123, + [{key: 'snippets/product-variant-picker.liquid', value: 'content'}], + session, + ) + + // Then + expect(waitForEnter).toHaveBeenCalledTimes(1) + expect(adminRequestDoc).toHaveBeenCalledTimes(2) + expect(results).toEqual([ + {key: 'snippets/product-variant-picker.liquid', success: true, operation: Operation.Upload}, + ]) + }) + + test('a non-challenge GraphQL error is not retried', async () => { + // Given + const error = new ClientError({status: 200, errors: [{message: 'boom'} as any]}, {query: ''}) + vi.mocked(adminRequestDoc).mockRejectedValue(error) + + // When + const promise = themePublish(123, session) + + // Then + await expect(promise).rejects.toBe(error) + expect(openURL).not.toHaveBeenCalled() + expect(adminRequestDoc).toHaveBeenCalledTimes(1) + }) +}) + describe('deleteThemeAssets', () => { test('deletes a theme asset', async () => { // Given @@ -940,6 +1020,21 @@ describe('parseThemeFileContent', () => { }) }) +function trustChallengeError(redirectTo: string): ClientError { + return new ClientError( + { + status: 200, + errors: [ + { + message: 'Challenge Required', + extensions: {code: 'CHALLENGE_REQUIRED', redirect_to: redirectTo}, + } as any, + ], + }, + {query: ''}, + ) +} + function themeAccessDeniedError(requiredAccess?: string, field = 'themes'): ClientError { const extensions = requiredAccess ? {code: 'ACCESS_DENIED', requiredAccess} : {code: 'ACCESS_DENIED'} const message = requiredAccess diff --git a/packages/cli-kit/src/public/node/themes/api.ts b/packages/cli-kit/src/public/node/themes/api.ts index ddbbdbc831f..7a892d0b87d 100644 --- a/packages/cli-kit/src/public/node/themes/api.ts +++ b/packages/cli-kit/src/public/node/themes/api.ts @@ -28,6 +28,7 @@ import {GetTheme} from '../../../cli/api/graphql/admin/generated/get_theme.js' import {FindDevelopmentThemeByName} from '../../../cli/api/graphql/admin/generated/find_development_theme_by_name.js' import {OnlineStorePasswordProtection} from '../../../cli/api/graphql/admin/generated/online_store_password_protection.js' import {extractGraphQLErrors} from '../../../private/node/api/graphql.js' +import {withTrustChallengeRetry} from '../../../private/node/themes/trust-challenge.js' import {RequestModeInput} from '../http.js' import {adminRequestDoc, type AdminRequestOptions} from '../api/admin.js' import {AdminSession} from '../session.js' @@ -629,7 +630,7 @@ async function requestThemeAdminDoc( options: AdminRequestOptions, ): Promise { try { - const response = await adminRequestDoc(options) + const response = await withTrustChallengeRetry(() => adminRequestDoc(options)) return response } catch (error) { abortIfMissingThemeAccessScope(error) From c3710cf469967d11f177269dca1400058bd50bc9 Mon Sep 17 00:00:00 2001 From: Iain Wilson Date: Wed, 14 Oct 2026 19:12:10 +0200 Subject: [PATCH 2/7] Handle Trust Battery challenges in CI, JSON mode, and theme dev - Abort instead of prompting in JSON output mode - Use copy that does not name the operation, and say that the browser must use the same account as the CLI - Render nothing for AbortSilentError in theme dev polling and upload errors - Cover @shopify/theme with the challenge changeset --- .changeset/theme-trust-challenge.md | 3 ++- .../node/themes/trust-challenge.test.ts | 21 +++++++++++++++- .../private/node/themes/trust-challenge.ts | 25 +++++++++---------- .../theme/src/cli/utilities/errors.test.ts | 12 ++++++++- packages/theme/src/cli/utilities/errors.ts | 3 ++- 5 files changed, 47 insertions(+), 17 deletions(-) diff --git a/.changeset/theme-trust-challenge.md b/.changeset/theme-trust-challenge.md index be0feb511cb..cb448195035 100644 --- a/.changeset/theme-trust-challenge.md +++ b/.changeset/theme-trust-challenge.md @@ -1,5 +1,6 @@ --- '@shopify/cli-kit': minor +'@shopify/theme': patch --- -Handle Trust Battery identity challenges for theme commands. When Shopify asks to verify your identity before a theme change, the CLI opens the verification page in your browser and retries once you complete it. +Handle Trust Battery identity challenges in theme commands. When Shopify asks you to verify your identity, the CLI opens the verification page in your browser and continues after you complete it. In CI, with `--no-input`, or with `--json`, the command stops and prints the verification link. diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts index bcc0d4271d1..548791d1f0d 100644 --- a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts @@ -1,5 +1,6 @@ import {trustChallengeUrl, withTrustChallengeRetry} from './trust-challenge.js' import {GraphQLClientError} from '../api/headers.js' +import {commandEventOutputMode} from '../command-event-context.js' import {waitForEnter} from '../ui/wait-for-enter.js' import {recordEvent} from '../../../public/node/analytics.js' import {adminFqdn} from '../../../public/node/context/fqdn.js' @@ -9,6 +10,7 @@ import {isTTY, renderInfo, renderSuccess} from '../../../public/node/ui.js' import {beforeEach, describe, expect, test, vi} from 'vitest' import {ClientError} from 'graphql-request' +vi.mock('../command-event-context.js') vi.mock('../ui/wait-for-enter.js') vi.mock('../../../public/node/analytics.js') vi.mock('../../../public/node/context/fqdn.js') @@ -20,6 +22,7 @@ const challengeUrl = 'https://admin.shopify.com/challenges/user_verification?tok beforeEach(() => { vi.mocked(adminFqdn).mockResolvedValue('admin.shopify.com') + vi.mocked(commandEventOutputMode).mockReturnValue(undefined) vi.mocked(isCI).mockReturnValue(false) vi.mocked(isTTY).mockReturnValue(true) vi.mocked(openURL).mockResolvedValue(true) @@ -121,7 +124,7 @@ describe('withTrustChallengeRetry', () => { expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:required') expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:browser-opened') expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:acknowledged') - expect(renderSuccess).toHaveBeenCalledWith({headline: 'Continuing with your theme changes.'}) + expect(renderSuccess).toHaveBeenCalledWith({headline: 'Verification complete. Continuing.'}) }) test('renders a link to the challenge URL when the browser cannot be opened', async () => { @@ -177,6 +180,22 @@ describe('withTrustChallengeRetry', () => { expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:non-interactive') }) + test('aborts with the challenge URL instead of prompting in JSON output mode', async () => { + vi.mocked(commandEventOutputMode).mockReturnValue('json') + const request = vi.fn().mockRejectedValue(challengeClientError(challengeUrl)) + + const promise = withTrustChallengeRetry(request) + + await expect(promise).rejects.toBeInstanceOf(AbortError) + await expect(promise).rejects.toMatchObject({ + nextSteps: [expect.arrayContaining([{link: {url: challengeUrl}}])], + }) + expect(request).toHaveBeenCalledTimes(1) + expect(openURL).not.toHaveBeenCalled() + expect(waitForEnter).not.toHaveBeenCalled() + expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:non-interactive') + }) + test('aborts instead of prompting when stdin is not a TTY', async () => { vi.mocked(isTTY).mockReturnValue(false) const request = vi.fn().mockRejectedValue(challengeClientError(challengeUrl)) diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.ts index 86eb012aa08..db04365debd 100644 --- a/packages/cli-kit/src/private/node/themes/trust-challenge.ts +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.ts @@ -1,4 +1,5 @@ import {GraphQLClientError} from '../api/headers.js' +import {commandEventOutputMode} from '../command-event-context.js' import {waitForEnter} from '../ui/wait-for-enter.js' import {recordEvent} from '../../../public/node/analytics.js' import {adminFqdn} from '../../../public/node/context/fqdn.js' @@ -8,7 +9,7 @@ import {isCI, openURL} from '../../../public/node/system.js' import {isTTY, renderInfo, renderSuccess} from '../../../public/node/ui.js' import {ClientError} from 'graphql-request' -export const TRUST_CHALLENGE_ERROR_CODE = 'CHALLENGE_REQUIRED' +const TRUST_CHALLENGE_ERROR_CODE = 'CHALLENGE_REQUIRED' interface GraphQLErrorWithExtensions { extensions?: { @@ -17,7 +18,7 @@ interface GraphQLErrorWithExtensions { } } -export interface TrustChallengeOptions { +interface TrustChallengeOptions { stdin?: typeof process.stdin } @@ -57,11 +58,9 @@ export async function withTrustChallengeRetry( if (!challengeUrl) throw error recordEvent('theme-api:trust-challenge:retry-challenged') - throw new AbortError( - 'Shopify still needs to verify your identity before it can apply this theme change.', - undefined, - [['Complete the verification at', {link: {url: challengeUrl}}, 'then run the command again.']], - ) + throw new AbortError('Shopify still needs to verify your identity before it can continue.', undefined, [ + ['Complete the verification at', {link: {url: challengeUrl}}, 'then run the command again.'], + ]) } } @@ -124,13 +123,13 @@ async function completeTrustChallenge(challengeUrl: string, options: TrustChalle async function promptForTrustChallenge(challengeUrl: string, options: TrustChallengeOptions): Promise { recordEvent('theme-api:trust-challenge:required') - if (isCI() || !isTTY()) { + if (isCI() || !isTTY() || commandEventOutputMode() === 'json') { recordEvent('theme-api:trust-challenge:non-interactive') - throw new AbortError('Shopify needs to verify your identity before it can apply this theme change.', undefined, [ + throw new AbortError('Shopify needs to verify your identity before it can continue.', undefined, [ [ 'Open', {link: {url: challengeUrl}}, - "in a browser where you're logged in to Shopify, complete the verification, then run the command again.", + "in a browser where you're logged in to Shopify with the same account you use in Shopify CLI, complete the verification, then run the command again.", ], ]) } @@ -144,10 +143,10 @@ async function promptForTrustChallenge(challengeUrl: string, options: TrustChall headline: 'Verify your identity to continue.', body: opened ? 'A verification page opened in your browser.' - : 'Shopify needs to verify your identity before it can apply your theme changes.', + : 'Shopify needs to verify your identity before it can continue.', link: opened ? undefined : {label: 'Open the verification page', url: challengeUrl}, nextSteps: [ - ['Complete the verification in your browser.'], + ['Complete the verification in your browser, logged in with the same account you use in Shopify CLI.'], ['Press', {userInput: 'Enter'}, 'here once you have completed it.'], ], orderedNextSteps: true, @@ -156,5 +155,5 @@ async function promptForTrustChallenge(challengeUrl: string, options: TrustChall await waitForEnter(options.stdin ?? process.stdin) recordEvent('theme-api:trust-challenge:acknowledged') - renderSuccess({headline: 'Continuing with your theme changes.'}) + renderSuccess({headline: 'Verification complete. Continuing.'}) } diff --git a/packages/theme/src/cli/utilities/errors.test.ts b/packages/theme/src/cli/utilities/errors.test.ts index 9d046017444..6fe28b5a5bd 100644 --- a/packages/theme/src/cli/utilities/errors.test.ts +++ b/packages/theme/src/cli/utilities/errors.test.ts @@ -6,9 +6,11 @@ import { extractFetchErrorInfo, } from './errors.js' import {describe, test, expect, vi, beforeEach, afterEach} from 'vitest' -import {AbortError} from '@shopify/cli-kit/node/error' +import {AbortError, AbortSilentError} from '@shopify/cli-kit/node/error' +import {recordError} from '@shopify/cli-kit/node/analytics' import {renderError, renderFatalError} from '@shopify/cli-kit/node/ui' +vi.mock('@shopify/cli-kit/node/analytics') vi.mock('@shopify/cli-kit/node/output') vi.mock('@shopify/cli-kit/node/ui') @@ -31,6 +33,14 @@ describe('errors', () => { body: 'test error', }) }) + + test('renders nothing for AbortSilentError', () => { + renderThrownError('Test Headline', new AbortSilentError()) + + expect(renderError).not.toHaveBeenCalled() + expect(renderFatalError).not.toHaveBeenCalled() + expect(recordError).not.toHaveBeenCalled() + }) }) describe('createSyncingCatchError', () => { diff --git a/packages/theme/src/cli/utilities/errors.ts b/packages/theme/src/cli/utilities/errors.ts index 03d98510df5..e52220125a4 100644 --- a/packages/theme/src/cli/utilities/errors.ts +++ b/packages/theme/src/cli/utilities/errors.ts @@ -1,5 +1,5 @@ import {commandEventOutputMode, emitCommandEvent} from '@shopify/cli-kit/node/command-events' -import {AbortError} from '@shopify/cli-kit/node/error' +import {AbortError, AbortSilentError} from '@shopify/cli-kit/node/error' import {outputDebug} from '@shopify/cli-kit/node/output' import {recordError} from '@shopify/cli-kit/node/analytics' import {renderError, renderFatalError} from '@shopify/cli-kit/node/ui' @@ -11,6 +11,7 @@ import {createError as createH3Error, type H3Error} from 'h3' * @param error - The error to render. */ export function renderThrownError(headline: string, error: Error | AbortError) { + if (error instanceof AbortSilentError) return recordError(error) if (commandEventOutputMode() === 'json') { From 61d5248f9003de801780eaf3b573ecdd4493dbf6 Mon Sep 17 00:00:00 2001 From: Iain Wilson Date: Wed, 14 Oct 2026 22:31:31 +0200 Subject: [PATCH 3/7] Show a neutral banner after Enter on a Trust Battery challenge The banner renders before the retry. The retry can still get a challenge. "Continuing." does not say that the verification is complete. --- .../cli-kit/src/private/node/themes/trust-challenge.test.ts | 2 +- packages/cli-kit/src/private/node/themes/trust-challenge.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts index 548791d1f0d..2e748c55e14 100644 --- a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts @@ -124,7 +124,7 @@ describe('withTrustChallengeRetry', () => { expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:required') expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:browser-opened') expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:acknowledged') - expect(renderSuccess).toHaveBeenCalledWith({headline: 'Verification complete. Continuing.'}) + expect(renderSuccess).toHaveBeenCalledWith({headline: 'Continuing.'}) }) test('renders a link to the challenge URL when the browser cannot be opened', async () => { diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.ts index db04365debd..1339649ea2c 100644 --- a/packages/cli-kit/src/private/node/themes/trust-challenge.ts +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.ts @@ -155,5 +155,5 @@ async function promptForTrustChallenge(challengeUrl: string, options: TrustChall await waitForEnter(options.stdin ?? process.stdin) recordEvent('theme-api:trust-challenge:acknowledged') - renderSuccess({headline: 'Verification complete. Continuing.'}) + renderSuccess({headline: 'Continuing.'}) } From 37a1abeba69001abf63ff48f6d070475c15047a5 Mon Sep 17 00:00:00 2001 From: Iain Wilson Date: Thu, 15 Oct 2026 15:10:07 +0200 Subject: [PATCH 4/7] Use the shared GraphQL error reader for Trust Battery challenges trustChallengeUrl now reads the errors with extractGraphQLErrors instead of its own copy. --- .../private/node/themes/trust-challenge.ts | 26 +++---------------- 1 file changed, 4 insertions(+), 22 deletions(-) diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.ts index 1339649ea2c..6ed17760c06 100644 --- a/packages/cli-kit/src/private/node/themes/trust-challenge.ts +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.ts @@ -1,4 +1,4 @@ -import {GraphQLClientError} from '../api/headers.js' +import {extractGraphQLErrors} from '../api/graphql.js' import {commandEventOutputMode} from '../command-event-context.js' import {waitForEnter} from '../ui/wait-for-enter.js' import {recordEvent} from '../../../public/node/analytics.js' @@ -7,17 +7,9 @@ import {AbortError} from '../../../public/node/error.js' import {outputDebug} from '../../../public/node/output.js' import {isCI, openURL} from '../../../public/node/system.js' import {isTTY, renderInfo, renderSuccess} from '../../../public/node/ui.js' -import {ClientError} from 'graphql-request' const TRUST_CHALLENGE_ERROR_CODE = 'CHALLENGE_REQUIRED' -interface GraphQLErrorWithExtensions { - extensions?: { - code?: unknown - redirect_to?: unknown - } -} - interface TrustChallengeOptions { stdin?: typeof process.stdin } @@ -74,9 +66,9 @@ export async function withTrustChallengeRetry( * @returns The challenge URL, or undefined when the error is not a trust challenge. */ export async function trustChallengeUrl(error: unknown): Promise { - const challengeError = graphQLErrors(error).find((graphQLError) => { - return graphQLError?.extensions?.code === TRUST_CHALLENGE_ERROR_CODE - }) + const challengeError = extractGraphQLErrors(error).find( + (graphQLError) => graphQLError.extensions?.code === TRUST_CHALLENGE_ERROR_CODE, + ) const redirectTo = challengeError?.extensions?.redirect_to if (typeof redirectTo !== 'string') return undefined @@ -88,16 +80,6 @@ export async function trustChallengeUrl(error: unknown): Promise { try { const parsedUrl = new URL(url) From db92f0f5d434bfe1c8396b57ce5a0ccffaadcd84 Mon Sep 17 00:00:00 2001 From: Iain Wilson Date: Fri, 16 Oct 2026 16:56:24 +0200 Subject: [PATCH 5/7] Address review: one prompt per verification, and stop app dev retries - A request that was already in flight when a challenge was completed retries without a second prompt. A counter of completed challenges is noted when each request starts, so a late challenge response for an earlier request does not open another browser tab. - Remove `TrustChallengeOptions.stdin`. Only one test used it, and `isTTY()` did not read it. `waitForEnter` uses its `process.stdin` default. - The two challenge aborts are a `TrustChallengeError` (an `AbortError`), and the themes API exports `isTrustChallengeError`. The `app dev` host theme retry loop rethrows it and `AbortSilentError`, so Ctrl+C or a failed verification stops theme creation instead of prompting up to four times. Other theme creation errors are still retried. --- .changeset/theme-trust-challenge.md | 3 +- .../theme/host-theme-manager.test.ts | 43 ++++++++++++++++++- .../extensions/theme/host-theme-manager.ts | 6 +-- .../node/themes/trust-challenge.test.ts | 41 ++++++++++++------ .../private/node/themes/trust-challenge.ts | 42 ++++++++++-------- .../cli-kit/src/public/node/themes/api.ts | 14 +++++- 6 files changed, 113 insertions(+), 36 deletions(-) diff --git a/.changeset/theme-trust-challenge.md b/.changeset/theme-trust-challenge.md index cb448195035..1854acedd66 100644 --- a/.changeset/theme-trust-challenge.md +++ b/.changeset/theme-trust-challenge.md @@ -1,6 +1,7 @@ --- '@shopify/cli-kit': minor '@shopify/theme': patch +'@shopify/app': patch --- -Handle Trust Battery identity challenges in theme commands. When Shopify asks you to verify your identity, the CLI opens the verification page in your browser and continues after you complete it. In CI, with `--no-input`, or with `--json`, the command stops and prints the verification link. +Handle Trust Battery identity challenges in theme commands. When Shopify asks you to verify your identity, the CLI opens the verification page in your browser and continues after you complete it. In CI, with `--no-input`, or with `--json`, the command stops and prints the verification link. `app dev` stops creating its host theme when you cancel the verification or it fails, instead of trying again. diff --git a/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.test.ts b/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.test.ts index 2524a1db019..cbb8ed20cc7 100644 --- a/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.test.ts +++ b/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.test.ts @@ -1,6 +1,7 @@ import {waitForThemeToBeProcessed} from './host-theme-watcher.js' import {HostThemeManager, DEFAULT_THEME_ZIP, FALLBACK_THEME_ZIP} from './host-theme-manager.js' -import {themeCreate} from '@shopify/cli-kit/node/themes/api' +import {isTrustChallengeError, themeCreate} from '@shopify/cli-kit/node/themes/api' +import {AbortError, AbortSilentError} from '@shopify/cli-kit/node/error' import {beforeEach, describe, expect, test, vi} from 'vitest' import {DEVELOPMENT_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils' import {AdminSession} from '@shopify/cli-kit/node/session' @@ -179,5 +180,45 @@ describe('HostThemeManager', () => { ) expect(themeCreate).toHaveBeenCalledTimes(4) }) + + test('stops retrying when the Trust Battery prompt is cancelled with Ctrl+C', async () => { + // Given + vi.mocked(themeCreate).mockRejectedValue(new AbortSilentError()) + + // When + // Then + await expect(themeManager.findOrCreate()).rejects.toBeInstanceOf(AbortSilentError) + expect(themeCreate).toHaveBeenCalledTimes(1) + }) + + test('stops retrying when a Trust Battery challenge ends the request', async () => { + // Given + const challengeError = new AbortError('Shopify still needs to verify your identity before it can continue.') + vi.mocked(themeCreate).mockRejectedValue(challengeError) + vi.mocked(isTrustChallengeError).mockImplementation((error) => error === challengeError) + + // When + // Then + await expect(themeManager.findOrCreate()).rejects.toBe(challengeError) + expect(themeCreate).toHaveBeenCalledTimes(1) + }) + + test('keeps retrying a theme creation abort that is not a Trust Battery challenge', async () => { + // Given + vi.mocked(themeCreate).mockRejectedValueOnce(new AbortError('Source is empty')).mockResolvedValueOnce({ + id: 12345, + name: 'Theme', + role: 'development', + createdAtRuntime: true, + processing: true, + }) + vi.mocked(isTrustChallengeError).mockReturnValue(false) + + // When + await themeManager.findOrCreate() + + // Then + expect(themeCreate).toHaveBeenCalledTimes(2) + }) }) }) diff --git a/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.ts b/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.ts index c80d08ee1d4..569da4c5d46 100644 --- a/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.ts +++ b/packages/app/src/cli/utilities/extensions/theme/host-theme-manager.ts @@ -3,9 +3,9 @@ import {getHostTheme, removeHostTheme, setHostTheme} from '@shopify/cli-kit/node import {ThemeManager} from '@shopify/cli-kit/node/themes/theme-manager' import {AdminSession} from '@shopify/cli-kit/node/session' import {Theme} from '@shopify/cli-kit/node/themes/types' -import {themeCreate} from '@shopify/cli-kit/node/themes/api' +import {isTrustChallengeError, themeCreate} from '@shopify/cli-kit/node/themes/api' import {DEVELOPMENT_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils' -import {BugError} from '@shopify/cli-kit/node/error' +import {AbortSilentError, BugError} from '@shopify/cli-kit/node/error' import {outputDebug} from '@shopify/cli-kit/node/output' export const DEFAULT_THEME_ZIP = 'https://codeload.github.com/Shopify/dawn/zip/refs/tags/v15.0.0' @@ -60,8 +60,8 @@ export class HostThemeManager extends ThemeManager { } else { throw new Error() } - // eslint-disable-next-line no-catch-all/no-catch-all } catch (error) { + if (error instanceof AbortSilentError || isTrustChallengeError(error)) throw error outputDebug(`Failed to create theme with name "${options.name}" and role "${options.role}". Retrying...`) } } diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts index 2e748c55e14..03328cefda1 100644 --- a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts @@ -1,4 +1,4 @@ -import {trustChallengeUrl, withTrustChallengeRetry} from './trust-challenge.js' +import {TrustChallengeError, trustChallengeUrl, withTrustChallengeRetry} from './trust-challenge.js' import {GraphQLClientError} from '../api/headers.js' import {commandEventOutputMode} from '../command-event-context.js' import {waitForEnter} from '../ui/wait-for-enter.js' @@ -147,7 +147,7 @@ describe('withTrustChallengeRetry', () => { const promise = withTrustChallengeRetry(request) - await expect(promise).rejects.toBeInstanceOf(AbortError) + await expect(promise).rejects.toBeInstanceOf(TrustChallengeError) await expect(promise).rejects.toMatchObject({ nextSteps: [expect.arrayContaining([{link: {url: challengeUrl}}])], }) @@ -171,7 +171,7 @@ describe('withTrustChallengeRetry', () => { const promise = withTrustChallengeRetry(request) - await expect(promise).rejects.toBeInstanceOf(AbortError) + await expect(promise).rejects.toBeInstanceOf(TrustChallengeError) await expect(promise).rejects.toMatchObject({ nextSteps: [expect.arrayContaining([{link: {url: challengeUrl}}])], }) @@ -249,6 +249,32 @@ describe('withTrustChallengeRetry', () => { expect(openURL).toHaveBeenCalledTimes(1) }) + test('retries without a second prompt when a request already in flight is challenged after the verification', async () => { + const enter = deferredEnter() + let rejectSecondResponse!: (error: unknown) => void + const secondResponse = new Promise((_resolve, reject) => { + rejectSecondResponse = reject + }) + const firstRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('first') + const secondRequest = vi.fn().mockReturnValueOnce(secondResponse).mockResolvedValueOnce('second') + + const firstResult = withTrustChallengeRetry(firstRequest) + const secondResult = withTrustChallengeRetry(secondRequest) + await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(1)) + enter.press() + await expect(firstResult).resolves.toBe('first') + + rejectSecondResponse(challengeClientError(challengeUrl)) + + await expect(secondResult).resolves.toBe('second') + expect(secondRequest).toHaveBeenCalledTimes(2) + expect(waitForEnter).toHaveBeenCalledTimes(1) + expect(openURL).toHaveBeenCalledTimes(1) + }) + test('rejects requests waiting on a pending challenge when the prompt is cancelled with Ctrl+C', async () => { const enter = deferredEnter() const challengedRequest = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)) @@ -265,15 +291,6 @@ describe('withTrustChallengeRetry', () => { expect(laterRequest).not.toHaveBeenCalled() expect(renderSuccess).not.toHaveBeenCalled() }) - - test('passes the provided stdin to the Enter prompt', async () => { - const stdin = {} as typeof process.stdin - const request = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)).mockResolvedValueOnce('retried') - - await withTrustChallengeRetry(request, {stdin}) - - expect(waitForEnter).toHaveBeenCalledWith(stdin) - }) }) function deferredEnter(): {press: () => void; cancel: () => void} { diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.ts index 6ed17760c06..a236114b74e 100644 --- a/packages/cli-kit/src/private/node/themes/trust-challenge.ts +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.ts @@ -10,11 +10,14 @@ import {isTTY, renderInfo, renderSuccess} from '../../../public/node/ui.js' const TRUST_CHALLENGE_ERROR_CODE = 'CHALLENGE_REQUIRED' -interface TrustChallengeOptions { - stdin?: typeof process.stdin -} +/** + * The abort that ends a Trust Battery challenge: the session cannot answer it, or + * the request was challenged again after the verification. + */ +export class TrustChallengeError extends AbortError {} let pendingChallenge: Promise | undefined +let completedChallenges = 0 /** * Runs a theme Admin API request and, when Core answers with a Trust Battery @@ -22,17 +25,15 @@ let pendingChallenge: Promise | undefined * * A request that starts while another request's challenge is still being completed * waits for that challenge first, so it is sent once instead of being challenged - * itself and then retried. + * itself and then retried. A request that was already in flight when a challenge + * was completed retries without a second prompt. * * @param request - The request to run and possibly retry. - * @param options - Overrides for the prompt, mainly for tests. * @returns The result of the request. */ -export async function withTrustChallengeRetry( - request: () => Promise, - options: TrustChallengeOptions = {}, -): Promise { +export async function withTrustChallengeRetry(request: () => Promise): Promise { if (pendingChallenge) await pendingChallenge + const completedChallengesAtStart = completedChallenges try { return await request() @@ -40,7 +41,8 @@ export async function withTrustChallengeRetry( const challengeUrl = await trustChallengeUrl(error) if (!challengeUrl) throw error - await completeTrustChallenge(challengeUrl, options) + const verifiedSinceStart = completedChallenges > completedChallengesAtStart + if (!verifiedSinceStart) await completeTrustChallenge(challengeUrl) } try { @@ -50,7 +52,7 @@ export async function withTrustChallengeRetry( if (!challengeUrl) throw error recordEvent('theme-api:trust-challenge:retry-challenged') - throw new AbortError('Shopify still needs to verify your identity before it can continue.', undefined, [ + throw new TrustChallengeError('Shopify still needs to verify your identity before it can continue.', undefined, [ ['Complete the verification at', {link: {url: challengeUrl}}, 'then run the command again.'], ]) } @@ -95,19 +97,23 @@ async function isAdminUrl(url: string): Promise { * all receive the same challenge. Share one prompt so the browser opens once and * every waiting request retries after the same verification. */ -async function completeTrustChallenge(challengeUrl: string, options: TrustChallengeOptions): Promise { - pendingChallenge ??= promptForTrustChallenge(challengeUrl, options).finally(() => { - pendingChallenge = undefined - }) +async function completeTrustChallenge(challengeUrl: string): Promise { + pendingChallenge ??= promptForTrustChallenge(challengeUrl) + .then(() => { + completedChallenges += 1 + }) + .finally(() => { + pendingChallenge = undefined + }) return pendingChallenge } -async function promptForTrustChallenge(challengeUrl: string, options: TrustChallengeOptions): Promise { +async function promptForTrustChallenge(challengeUrl: string): Promise { recordEvent('theme-api:trust-challenge:required') if (isCI() || !isTTY() || commandEventOutputMode() === 'json') { recordEvent('theme-api:trust-challenge:non-interactive') - throw new AbortError('Shopify needs to verify your identity before it can continue.', undefined, [ + throw new TrustChallengeError('Shopify needs to verify your identity before it can continue.', undefined, [ [ 'Open', {link: {url: challengeUrl}}, @@ -134,7 +140,7 @@ async function promptForTrustChallenge(challengeUrl: string, options: TrustChall orderedNextSteps: true, }) - await waitForEnter(options.stdin ?? process.stdin) + await waitForEnter() recordEvent('theme-api:trust-challenge:acknowledged') renderSuccess({headline: 'Continuing.'}) diff --git a/packages/cli-kit/src/public/node/themes/api.ts b/packages/cli-kit/src/public/node/themes/api.ts index 7a892d0b87d..9ec1e8100b8 100644 --- a/packages/cli-kit/src/public/node/themes/api.ts +++ b/packages/cli-kit/src/public/node/themes/api.ts @@ -28,7 +28,7 @@ import {GetTheme} from '../../../cli/api/graphql/admin/generated/get_theme.js' import {FindDevelopmentThemeByName} from '../../../cli/api/graphql/admin/generated/find_development_theme_by_name.js' import {OnlineStorePasswordProtection} from '../../../cli/api/graphql/admin/generated/online_store_password_protection.js' import {extractGraphQLErrors} from '../../../private/node/api/graphql.js' -import {withTrustChallengeRetry} from '../../../private/node/themes/trust-challenge.js' +import {TrustChallengeError, withTrustChallengeRetry} from '../../../private/node/themes/trust-challenge.js' import {RequestModeInput} from '../http.js' import {adminRequestDoc, type AdminRequestOptions} from '../api/admin.js' import {AdminSession} from '../session.js' @@ -626,6 +626,18 @@ function unexpectedGraphQLError(message: string): never { throw recordError(new AbortError(message)) } +/** + * Whether the error ends a Trust Battery challenge on a theme request. A caller that + * retries theme requests must rethrow it, or the user is asked to verify again for + * each attempt. + * + * @param error - The error to check. + * @returns True when the error ends a Trust Battery challenge. + */ +export function isTrustChallengeError(error: unknown): boolean { + return error instanceof TrustChallengeError +} + async function requestThemeAdminDoc( options: AdminRequestOptions, ): Promise { From 260c8651e7ab7d2418cb42b8c35b0ab105a29b45 Mon Sep 17 00:00:00 2001 From: Iain Wilson Date: Sun, 18 Oct 2026 20:10:27 +0200 Subject: [PATCH 6/7] Wait for an open Trust Battery prompt before retrying an older request A request that was in flight during an earlier verification retried at once when its challenge arrived, even if a newer request had opened another prompt in the meantime. That retry was challenged again and aborted while the prompt was still waiting for Enter. Such a request now waits for the open prompt first. --- .../node/themes/trust-challenge.test.ts | 37 +++++++++++++++++++ .../private/node/themes/trust-challenge.ts | 5 ++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts index 03328cefda1..c4e32dfa3b3 100644 --- a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts @@ -275,6 +275,43 @@ describe('withTrustChallengeRetry', () => { expect(openURL).toHaveBeenCalledTimes(1) }) + test('waits for a newer open prompt before retrying a request that was in flight during an earlier verification', async () => { + const firstEnter = Promise.withResolvers() + const secondEnter = Promise.withResolvers() + vi.mocked(waitForEnter).mockReturnValueOnce(firstEnter.promise).mockReturnValueOnce(secondEnter.promise) + const secondResponse = Promise.withResolvers() + const firstRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('first') + const secondRequest = vi.fn().mockReturnValueOnce(secondResponse.promise).mockResolvedValueOnce('second') + const thirdRequest = vi + .fn() + .mockRejectedValueOnce(challengeClientError(challengeUrl)) + .mockResolvedValueOnce('third') + + const firstResult = withTrustChallengeRetry(firstRequest) + const secondResult = withTrustChallengeRetry(secondRequest) + await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(1)) + firstEnter.resolve() + await expect(firstResult).resolves.toBe('first') + + const thirdResult = withTrustChallengeRetry(thirdRequest) + await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(2)) + + secondResponse.reject(challengeClientError(challengeUrl)) + // No timer is involved: the mocks resolve at once, so one event-loop turn runs the + // second request's whole challenge check before the assertion. + await new Promise((resolve) => setImmediate(resolve)) + expect(secondRequest).toHaveBeenCalledTimes(1) + + secondEnter.resolve() + + await expect(Promise.all([secondResult, thirdResult])).resolves.toEqual(['second', 'third']) + expect(secondRequest).toHaveBeenCalledTimes(2) + expect(waitForEnter).toHaveBeenCalledTimes(2) + }) + test('rejects requests waiting on a pending challenge when the prompt is cancelled with Ctrl+C', async () => { const enter = deferredEnter() const challengedRequest = vi.fn().mockRejectedValueOnce(challengeClientError(challengeUrl)) diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.ts index a236114b74e..342e4ea650b 100644 --- a/packages/cli-kit/src/private/node/themes/trust-challenge.ts +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.ts @@ -26,7 +26,8 @@ let completedChallenges = 0 * A request that starts while another request's challenge is still being completed * waits for that challenge first, so it is sent once instead of being challenged * itself and then retried. A request that was already in flight when a challenge - * was completed retries without a second prompt. + * was completed retries without a second prompt, unless another prompt is open: + * then it waits for that prompt too. * * @param request - The request to run and possibly retry. * @returns The result of the request. @@ -42,7 +43,7 @@ export async function withTrustChallengeRetry(request: () => Promise): Pro if (!challengeUrl) throw error const verifiedSinceStart = completedChallenges > completedChallengesAtStart - if (!verifiedSinceStart) await completeTrustChallenge(challengeUrl) + if (pendingChallenge || !verifiedSinceStart) await completeTrustChallenge(challengeUrl) } try { From 6f43f013fdfe2895a141a373199bb9e097e51070 Mon Sep 17 00:00:00 2001 From: Iain Wilson Date: Tue, 20 Oct 2026 14:14:04 +0200 Subject: [PATCH 7/7] Address review: share request attempts and reuse keypress - withTrustChallengeRetry runs both attempts through one helper, attemptRequest. It returns the result or the challenge URL and throws any other error. - keypress accepts a keys option. Only those keys end the wait. It sets raw mode back to its earlier state and reads chunks as text, so it also detects Ctrl+C when Ink has set the stdin encoding. The Trust Battery prompt calls keypress with Enter and Space, and the waitForEnter helper is removed. --- .changeset/keypress-accepted-keys.md | 5 + .../node/themes/trust-challenge.test.ts | 37 ++-- .../private/node/themes/trust-challenge.ts | 43 +++-- .../private/node/ui/wait-for-enter.test.ts | 161 ------------------ .../src/private/node/ui/wait-for-enter.ts | 49 ------ .../src/public/node/themes/api.test.ts | 10 +- packages/cli-kit/src/public/node/ui.test.ts | 159 ++++++++++++----- packages/cli-kit/src/public/node/ui.tsx | 65 +++++-- 8 files changed, 220 insertions(+), 309 deletions(-) create mode 100644 .changeset/keypress-accepted-keys.md delete mode 100644 packages/cli-kit/src/private/node/ui/wait-for-enter.test.ts delete mode 100644 packages/cli-kit/src/private/node/ui/wait-for-enter.ts diff --git a/.changeset/keypress-accepted-keys.md b/.changeset/keypress-accepted-keys.md new file mode 100644 index 00000000000..aef7cdc7589 --- /dev/null +++ b/.changeset/keypress-accepted-keys.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': minor +--- + +`keypress` accepts a `keys` option: only those keys end the wait, and other keys are ignored. `keypress` now sets raw mode back to its earlier state, and it also detects Ctrl+C when an Ink renderer has set the stdin encoding. diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts index c4e32dfa3b3..518c8507c8d 100644 --- a/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.test.ts @@ -1,17 +1,15 @@ import {TrustChallengeError, trustChallengeUrl, withTrustChallengeRetry} from './trust-challenge.js' import {GraphQLClientError} from '../api/headers.js' import {commandEventOutputMode} from '../command-event-context.js' -import {waitForEnter} from '../ui/wait-for-enter.js' import {recordEvent} from '../../../public/node/analytics.js' import {adminFqdn} from '../../../public/node/context/fqdn.js' import {AbortError, AbortSilentError} from '../../../public/node/error.js' import {isCI, openURL} from '../../../public/node/system.js' -import {isTTY, renderInfo, renderSuccess} from '../../../public/node/ui.js' +import {isTTY, keypress, renderInfo, renderSuccess} from '../../../public/node/ui.js' import {beforeEach, describe, expect, test, vi} from 'vitest' import {ClientError} from 'graphql-request' vi.mock('../command-event-context.js') -vi.mock('../ui/wait-for-enter.js') vi.mock('../../../public/node/analytics.js') vi.mock('../../../public/node/context/fqdn.js') vi.mock('../../../public/node/output.js') @@ -26,7 +24,7 @@ beforeEach(() => { vi.mocked(isCI).mockReturnValue(false) vi.mocked(isTTY).mockReturnValue(true) vi.mocked(openURL).mockResolvedValue(true) - vi.mocked(waitForEnter).mockResolvedValue(undefined) + vi.mocked(keypress).mockResolvedValue(undefined) }) describe('trustChallengeUrl', () => { @@ -94,7 +92,7 @@ describe('withTrustChallengeRetry', () => { expect(request).toHaveBeenCalledTimes(1) expect(openURL).not.toHaveBeenCalled() - expect(waitForEnter).not.toHaveBeenCalled() + expect(keypress).not.toHaveBeenCalled() }) test('rethrows an error that is not a trust challenge without prompting', async () => { @@ -114,7 +112,8 @@ describe('withTrustChallengeRetry', () => { expect(request).toHaveBeenCalledTimes(2) expect(openURL).toHaveBeenCalledWith(challengeUrl) - expect(waitForEnter).toHaveBeenCalledTimes(1) + // Only Enter or Space answer the prompt: `theme dev` binds other keys, such as `t`, on the same stdin. + expect(keypress).toHaveBeenCalledExactlyOnceWith(process.stdin, undefined, {keys: ['\r', '\n', ' ']}) expect(renderInfo).toHaveBeenCalledWith( expect.objectContaining({ headline: 'Verify your identity to continue.', @@ -152,7 +151,7 @@ describe('withTrustChallengeRetry', () => { nextSteps: [expect.arrayContaining([{link: {url: challengeUrl}}])], }) expect(request).toHaveBeenCalledTimes(2) - expect(waitForEnter).toHaveBeenCalledTimes(1) + expect(keypress).toHaveBeenCalledTimes(1) expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:retry-challenged') }) @@ -192,7 +191,7 @@ describe('withTrustChallengeRetry', () => { }) expect(request).toHaveBeenCalledTimes(1) expect(openURL).not.toHaveBeenCalled() - expect(waitForEnter).not.toHaveBeenCalled() + expect(keypress).not.toHaveBeenCalled() expect(recordEvent).toHaveBeenCalledWith('theme-api:trust-challenge:non-interactive') }) @@ -202,7 +201,7 @@ describe('withTrustChallengeRetry', () => { await expect(withTrustChallengeRetry(request)).rejects.toBeInstanceOf(AbortError) - expect(waitForEnter).not.toHaveBeenCalled() + expect(keypress).not.toHaveBeenCalled() }) test('shares a single prompt across concurrent requests that are challenged together', async () => { @@ -217,7 +216,7 @@ describe('withTrustChallengeRetry', () => { .mockResolvedValueOnce('second') const results = Promise.all([withTrustChallengeRetry(firstRequest), withTrustChallengeRetry(secondRequest)]) - await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(1)) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(1)) enter.press() await expect(results).resolves.toEqual(['first', 'second']) @@ -236,7 +235,7 @@ describe('withTrustChallengeRetry', () => { const laterRequest = vi.fn().mockResolvedValue('later') const challengedResult = withTrustChallengeRetry(challengedRequest) - await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(1)) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(1)) const laterResult = withTrustChallengeRetry(laterRequest) await Promise.resolve() @@ -263,7 +262,7 @@ describe('withTrustChallengeRetry', () => { const firstResult = withTrustChallengeRetry(firstRequest) const secondResult = withTrustChallengeRetry(secondRequest) - await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(1)) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(1)) enter.press() await expect(firstResult).resolves.toBe('first') @@ -271,14 +270,14 @@ describe('withTrustChallengeRetry', () => { await expect(secondResult).resolves.toBe('second') expect(secondRequest).toHaveBeenCalledTimes(2) - expect(waitForEnter).toHaveBeenCalledTimes(1) + expect(keypress).toHaveBeenCalledTimes(1) expect(openURL).toHaveBeenCalledTimes(1) }) test('waits for a newer open prompt before retrying a request that was in flight during an earlier verification', async () => { const firstEnter = Promise.withResolvers() const secondEnter = Promise.withResolvers() - vi.mocked(waitForEnter).mockReturnValueOnce(firstEnter.promise).mockReturnValueOnce(secondEnter.promise) + vi.mocked(keypress).mockReturnValueOnce(firstEnter.promise).mockReturnValueOnce(secondEnter.promise) const secondResponse = Promise.withResolvers() const firstRequest = vi .fn() @@ -292,12 +291,12 @@ describe('withTrustChallengeRetry', () => { const firstResult = withTrustChallengeRetry(firstRequest) const secondResult = withTrustChallengeRetry(secondRequest) - await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(1)) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(1)) firstEnter.resolve() await expect(firstResult).resolves.toBe('first') const thirdResult = withTrustChallengeRetry(thirdRequest) - await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(2)) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(2)) secondResponse.reject(challengeClientError(challengeUrl)) // No timer is involved: the mocks resolve at once, so one event-loop turn runs the @@ -309,7 +308,7 @@ describe('withTrustChallengeRetry', () => { await expect(Promise.all([secondResult, thirdResult])).resolves.toEqual(['second', 'third']) expect(secondRequest).toHaveBeenCalledTimes(2) - expect(waitForEnter).toHaveBeenCalledTimes(2) + expect(keypress).toHaveBeenCalledTimes(2) }) test('rejects requests waiting on a pending challenge when the prompt is cancelled with Ctrl+C', async () => { @@ -318,7 +317,7 @@ describe('withTrustChallengeRetry', () => { const laterRequest = vi.fn().mockResolvedValue('later') const challengedResult = withTrustChallengeRetry(challengedRequest) - await vi.waitFor(() => expect(waitForEnter).toHaveBeenCalledTimes(1)) + await vi.waitFor(() => expect(keypress).toHaveBeenCalledTimes(1)) const laterResult = withTrustChallengeRetry(laterRequest) enter.cancel() @@ -333,7 +332,7 @@ describe('withTrustChallengeRetry', () => { function deferredEnter(): {press: () => void; cancel: () => void} { let press!: () => void let cancel!: () => void - vi.mocked(waitForEnter).mockReturnValue( + vi.mocked(keypress).mockReturnValue( new Promise((resolve, reject) => { press = resolve cancel = () => reject(new AbortSilentError()) diff --git a/packages/cli-kit/src/private/node/themes/trust-challenge.ts b/packages/cli-kit/src/private/node/themes/trust-challenge.ts index 342e4ea650b..255457c5720 100644 --- a/packages/cli-kit/src/private/node/themes/trust-challenge.ts +++ b/packages/cli-kit/src/private/node/themes/trust-challenge.ts @@ -1,14 +1,15 @@ import {extractGraphQLErrors} from '../api/graphql.js' import {commandEventOutputMode} from '../command-event-context.js' -import {waitForEnter} from '../ui/wait-for-enter.js' import {recordEvent} from '../../../public/node/analytics.js' import {adminFqdn} from '../../../public/node/context/fqdn.js' import {AbortError} from '../../../public/node/error.js' import {outputDebug} from '../../../public/node/output.js' import {isCI, openURL} from '../../../public/node/system.js' -import {isTTY, renderInfo, renderSuccess} from '../../../public/node/ui.js' +import {isTTY, keypress, renderInfo, renderSuccess} from '../../../public/node/ui.js' const TRUST_CHALLENGE_ERROR_CODE = 'CHALLENGE_REQUIRED' +// Enter or Space. `theme dev` binds other single keys on the same stdin to its own shortcuts. +const CONTINUE_KEYS = ['\r', '\n', ' '] /** * The abort that ends a Trust Battery challenge: the session cannot answer it, or @@ -36,26 +37,34 @@ export async function withTrustChallengeRetry(request: () => Promise): Pro if (pendingChallenge) await pendingChallenge const completedChallengesAtStart = completedChallenges - try { - return await request() - } catch (error) { - const challengeUrl = await trustChallengeUrl(error) - if (!challengeUrl) throw error + const attempt = await attemptRequest(request) + if (!attempt.challenged) return attempt.result - const verifiedSinceStart = completedChallenges > completedChallengesAtStart - if (pendingChallenge || !verifiedSinceStart) await completeTrustChallenge(challengeUrl) - } + const verifiedSinceStart = completedChallenges > completedChallengesAtStart + if (pendingChallenge || !verifiedSinceStart) await completeTrustChallenge(attempt.challengeUrl) + + const retry = await attemptRequest(request) + if (!retry.challenged) return retry.result + recordEvent('theme-api:trust-challenge:retry-challenged') + throw new TrustChallengeError('Shopify still needs to verify your identity before it can continue.', undefined, [ + ['Complete the verification at', {link: {url: retry.challengeUrl}}, 'then run the command again.'], + ]) +} + +type RequestAttempt = {challenged: false; result: T} | {challenged: true; challengeUrl: string} + +/** + * Runs the request once. A Trust Battery challenge becomes a result with the challenge URL; + * any other error is thrown. + */ +async function attemptRequest(request: () => Promise): Promise> { try { - return await request() + return {challenged: false, result: await request()} } catch (error) { const challengeUrl = await trustChallengeUrl(error) if (!challengeUrl) throw error - - recordEvent('theme-api:trust-challenge:retry-challenged') - throw new TrustChallengeError('Shopify still needs to verify your identity before it can continue.', undefined, [ - ['Complete the verification at', {link: {url: challengeUrl}}, 'then run the command again.'], - ]) + return {challenged: true, challengeUrl} } } @@ -141,7 +150,7 @@ async function promptForTrustChallenge(challengeUrl: string): Promise { orderedNextSteps: true, }) - await waitForEnter() + await keypress(process.stdin, undefined, {keys: CONTINUE_KEYS}) recordEvent('theme-api:trust-challenge:acknowledged') renderSuccess({headline: 'Continuing.'}) diff --git a/packages/cli-kit/src/private/node/ui/wait-for-enter.test.ts b/packages/cli-kit/src/private/node/ui/wait-for-enter.test.ts deleted file mode 100644 index 1c80f4ae7fe..00000000000 --- a/packages/cli-kit/src/private/node/ui/wait-for-enter.test.ts +++ /dev/null @@ -1,161 +0,0 @@ -import {waitForEnter} from './wait-for-enter.js' -import {AbortSilentError} from '../../../public/node/error.js' -import {describe, expect, test, vi} from 'vitest' -import {EventEmitter} from 'events' - -class FakeStdin extends EventEmitter { - isRaw: boolean - setRawMode = vi.fn((mode: boolean) => { - this.isRaw = mode - return this - }) - - ref = vi.fn(() => this) - unref = vi.fn(() => this) - - constructor({isRaw}: {isRaw: boolean}) { - super() - this.isRaw = isRaw - } - - press(key: string): void { - this.emit('data', Buffer.from(key)) - } - - pressEncoded(key: string): void { - this.emit('data', key) - } - - asStdin(): typeof process.stdin { - return this as unknown as typeof process.stdin - } -} - -describe('waitForEnter', () => { - test('resolves when Enter is pressed as a carriage return', async () => { - const stdin = new FakeStdin({isRaw: false}) - const promise = waitForEnter(stdin.asStdin()) - - stdin.press('\r') - - await expect(promise).resolves.toBeUndefined() - }) - - test('resolves when Enter is pressed as a line feed', async () => { - const stdin = new FakeStdin({isRaw: false}) - const promise = waitForEnter(stdin.asStdin()) - - stdin.press('\n') - - await expect(promise).resolves.toBeUndefined() - }) - - test('resolves when Space is pressed', async () => { - const stdin = new FakeStdin({isRaw: false}) - const promise = waitForEnter(stdin.asStdin()) - - stdin.press(' ') - - await expect(promise).resolves.toBeUndefined() - }) - - test('ignores other keys until Enter is pressed', async () => { - const stdin = new FakeStdin({isRaw: false}) - const settled = vi.fn() - const promise = waitForEnter(stdin.asStdin()).then(settled) - - stdin.press('t') - stdin.press('p') - await Promise.resolve() - expect(settled).not.toHaveBeenCalled() - - stdin.press('\r') - - await expect(promise).resolves.toBeUndefined() - }) - - test('rejects with AbortSilentError when Ctrl+C is pressed', async () => { - const stdin = new FakeStdin({isRaw: false}) - const promise = waitForEnter(stdin.asStdin()) - - stdin.press('\u0003') - - await expect(promise).rejects.toBeInstanceOf(AbortSilentError) - }) - - test('resolves when Enter arrives as a utf8 string chunk because another renderer set the stdin encoding', async () => { - const stdin = new FakeStdin({isRaw: true}) - const promise = waitForEnter(stdin.asStdin()) - - stdin.pressEncoded('\r') - - await expect(promise).resolves.toBeUndefined() - }) - - test('ignores other keys that arrive as utf8 string chunks', async () => { - const stdin = new FakeStdin({isRaw: true}) - const settled = vi.fn() - const promise = waitForEnter(stdin.asStdin()).then(settled) - - stdin.pressEncoded('t') - await Promise.resolve() - expect(settled).not.toHaveBeenCalled() - - stdin.pressEncoded('\n') - - await expect(promise).resolves.toBeUndefined() - }) - - test('rejects with AbortSilentError when Ctrl+C arrives as a utf8 string chunk', async () => { - const stdin = new FakeStdin({isRaw: true}) - const promise = waitForEnter(stdin.asStdin()) - - stdin.pressEncoded('\u0003') - - await expect(promise).rejects.toBeInstanceOf(AbortSilentError) - }) - - test('resolves when Enter is part of a multi-character chunk', async () => { - const stdin = new FakeStdin({isRaw: false}) - const promise = waitForEnter(stdin.asStdin()) - - stdin.press('ab\r') - - await expect(promise).resolves.toBeUndefined() - }) - - test('turns raw mode on while waiting and back off afterwards when stdin was not raw', async () => { - const stdin = new FakeStdin({isRaw: false}) - const promise = waitForEnter(stdin.asStdin()) - - expect(stdin.setRawMode).toHaveBeenCalledWith(true) - stdin.press('\r') - await promise - - expect(stdin.setRawMode).toHaveBeenLastCalledWith(false) - }) - - test('leaves raw mode alone when stdin was already raw', async () => { - const stdin = new FakeStdin({isRaw: true}) - const promise = waitForEnter(stdin.asStdin()) - - stdin.press('\r') - await promise - - expect(stdin.setRawMode).not.toHaveBeenCalled() - expect(stdin.isRaw).toBe(true) - }) - - test('stops listening to stdin and unrefs it once settled', async () => { - const stdin = new FakeStdin({isRaw: true}) - const promise = waitForEnter(stdin.asStdin()) - - expect(stdin.ref).toHaveBeenCalledTimes(1) - expect(stdin.listenerCount('data')).toBe(1) - stdin.press('\r') - await promise - - expect(stdin.listenerCount('data')).toBe(0) - expect(stdin.unref).toHaveBeenCalledTimes(1) - }) -}) diff --git a/packages/cli-kit/src/private/node/ui/wait-for-enter.ts b/packages/cli-kit/src/private/node/ui/wait-for-enter.ts deleted file mode 100644 index c1afa6dfc1f..00000000000 --- a/packages/cli-kit/src/private/node/ui/wait-for-enter.ts +++ /dev/null @@ -1,49 +0,0 @@ -import {AbortSilentError} from '../../../public/node/error.js' -import {runWithTimer} from '../../../public/node/metadata.js' - -const CTRL_C = '\u0003' -const ACCEPTED_KEYS = new Set(['\r', '\n', ' ']) - -/** - * Waits for Enter (or Space) on stdin and ignores every other key. Ctrl+C rejects - * with AbortSilentError so the process exits the same way it would from any prompt. - * - * Raw mode is left the way it was found. Long-running commands such as `theme dev` - * keep stdin raw for their own single-key shortcuts, and the accepted keys are ones - * those commands do not bind, so pressing Enter here never triggers a shortcut. - * - * Chunks are compared as text rather than bytes: an active Ink renderer (for example - * the `theme push` progress bar) calls `stdin.setEncoding('utf8')`, after which stdin - * emits strings instead of Buffers. - * - * @param stdin - The stream to read from; defaults to process.stdin. - * @returns A promise that resolves when Enter or Space is pressed. - */ -export function waitForEnter(stdin: typeof process.stdin = process.stdin): Promise { - return runWithTimer('cmd_all_timing_prompts_ms')(() => { - return new Promise((resolve, reject) => { - const stdinWasRaw = stdin.isRaw === true - - const cleanup = () => { - stdin.off('data', onData) - if (!stdinWasRaw) stdin.setRawMode(false) - stdin.unref() - } - - const onData = (chunk: Buffer | string) => { - const text = typeof chunk === 'string' ? chunk : chunk.toString('utf8') - if (text.includes(CTRL_C)) { - cleanup() - reject(new AbortSilentError()) - } else if ([...text].some((character) => ACCEPTED_KEYS.has(character))) { - cleanup() - resolve() - } - } - - if (!stdinWasRaw) stdin.setRawMode(true) - stdin.ref() - stdin.on('data', onData) - }) - }) -} diff --git a/packages/cli-kit/src/public/node/themes/api.test.ts b/packages/cli-kit/src/public/node/themes/api.test.ts index 34b9b43f165..4e2faaa8d65 100644 --- a/packages/cli-kit/src/public/node/themes/api.test.ts +++ b/packages/cli-kit/src/public/node/themes/api.test.ts @@ -33,8 +33,7 @@ import {adminRequestDoc, supportedApiVersions} from '../api/admin.js' import {AbortError} from '../error.js' import {adminFqdn} from '../context/fqdn.js' import {openURL} from '../system.js' -import {isTTY} from '../ui.js' -import {waitForEnter} from '../../../private/node/ui/wait-for-enter.js' +import {isTTY, keypress} from '../ui.js' import {test, vi, expect, describe, beforeEach} from 'vitest' import {ClientError} from 'graphql-request' @@ -42,7 +41,6 @@ import {ClientError} from 'graphql-request' vi.mock('../api/admin.js') vi.mock('../context/fqdn.js') vi.mock('../ui.js') -vi.mock('../../../private/node/ui/wait-for-enter.js') vi.mock('@shopify/cli-kit/node/system') vi.stubGlobal('fetch', vi.fn()) @@ -576,7 +574,7 @@ describe('trust challenges', () => { vi.mocked(adminFqdn).mockResolvedValue('admin.shopify.com') vi.mocked(isTTY).mockReturnValue(true) vi.mocked(openURL).mockResolvedValue(true) - vi.mocked(waitForEnter).mockResolvedValue(undefined) + vi.mocked(keypress).mockResolvedValue(undefined) }) test('themePublish opens the challenge in the browser and retries after the user completes it', async () => { @@ -596,7 +594,7 @@ describe('trust challenges', () => { // Then expect(openURL).toHaveBeenCalledWith(challengeUrl) - expect(waitForEnter).toHaveBeenCalledTimes(1) + expect(keypress).toHaveBeenCalledTimes(1) expect(adminRequestDoc).toHaveBeenCalledTimes(2) expect(theme!.id).toEqual(id) }) @@ -620,7 +618,7 @@ describe('trust challenges', () => { ) // Then - expect(waitForEnter).toHaveBeenCalledTimes(1) + expect(keypress).toHaveBeenCalledTimes(1) expect(adminRequestDoc).toHaveBeenCalledTimes(2) expect(results).toEqual([ {key: 'snippets/product-variant-picker.liquid', success: true, operation: Operation.Upload}, diff --git a/packages/cli-kit/src/public/node/ui.test.ts b/packages/cli-kit/src/public/node/ui.test.ts index dc299221e1c..d38962e4acf 100644 --- a/packages/cli-kit/src/public/node/ui.test.ts +++ b/packages/cli-kit/src/public/node/ui.test.ts @@ -14,7 +14,7 @@ import { Task, } from './ui.js' import {AbortSignal} from './abort.js' -import {BugError, FatalError, AbortError, FatalErrorType} from './error.js' +import {BugError, FatalError, AbortError, AbortSilentError} from './error.js' import {renderCommandEventAsJson, runWithCommandEvents, runWithCommandEventsForCommand} from './command-events.js' import {mockAndCaptureOutput, withCapturedStandardStreams} from './testing/output.js' import {outputResult, TokenizedString, unstyled} from './output.js' @@ -23,6 +23,8 @@ import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' import supportsHyperlinks from 'supports-hyperlinks' import {Writable} from 'stream' +import {EventEmitter} from 'events' +import {setImmediate} from 'timers/promises' import type {RenderOptions} from 'ink' vi.mock('supports-hyperlinks') @@ -744,54 +746,131 @@ describe('renderTasks', async () => { }) }) +class FakeStdin extends EventEmitter { + isRaw: boolean + setRawMode = vi.fn((mode: boolean) => { + this.isRaw = mode + return this + }) + + ref = vi.fn(() => this) + unref = vi.fn(() => this) + + constructor({isRaw}: {isRaw: boolean}) { + super() + this.isRaw = isRaw + } + + press(key: string): void { + this.emit('data', Buffer.from(key)) + } + + // An active Ink renderer calls `setEncoding('utf8')`, and then stdin emits strings. + pressEncoded(key: string): void { + this.emit('data', key) + } + + asStdin(): typeof process.stdin { + return this as unknown as typeof process.stdin + } +} + describe('keypress', async () => { - test('waits for a keypress, managing stdin', async () => { - let registeredListener: any - const mockStdin = { - setRawMode: vi.fn(), - once: (event: string, callback: any) => { - registeredListener = callback - }, - ref: vi.fn(), - unref: vi.fn(), - } as any + const enterKeys = {keys: ['\r', '\n', ' ']} + + test('resolves on any key when no keys are given', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}) - const promise = keypress(mockStdin, {skipTTYCheck: true}) - expect(mockStdin.ref).toBeCalled() - expect(mockStdin.setRawMode).toHaveBeenLastCalledWith(true) - // create a buffer representing pressing the enter key - registeredListener(Buffer.from([13])) + stdin.press('x') + + await expect(promise).resolves.toBeUndefined() + }) + + test.each([ + ['a carriage return', '\r'], + ['a line feed', '\n'], + ['a space', ' '], + ['a multi-character chunk that contains an accepted key', 'ab\r'], + ])('resolves on %s when it is an accepted key', async (_name, key) => { + const stdin = new FakeStdin({isRaw: false}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}, enterKeys) + + stdin.press(key) + + await expect(promise).resolves.toBeUndefined() + }) + + test('ignores keys that are not accepted', async () => { + const stdin = new FakeStdin({isRaw: false}) + const settled = vi.fn() + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}, enterKeys).then(settled) + + stdin.press('t') + stdin.pressEncoded('p') + await setImmediate() + expect(settled).not.toHaveBeenCalled() + + stdin.press('\r') + + await expect(promise).resolves.toBeUndefined() + }) + test('resolves when an accepted key arrives as a string chunk', async () => { + const stdin = new FakeStdin({isRaw: true}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}, enterKeys) + + stdin.pressEncoded('\r') + + await expect(promise).resolves.toBeUndefined() + }) + + test.each([ + ['a Buffer', (stdin: FakeStdin) => stdin.press('\u0003')], + ['a string chunk', (stdin: FakeStdin) => stdin.pressEncoded('\u0003')], + ])('rejects with AbortSilentError when Ctrl+C arrives as %s', async (_name, pressCtrlC) => { + const stdin = new FakeStdin({isRaw: false}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}, enterKeys) + + pressCtrlC(stdin) + + await expect(promise).rejects.toBeInstanceOf(AbortSilentError) + expect(stdin.listenerCount('data')).toBe(0) + }) + + test('turns raw mode on while it waits and off afterwards when stdin was not raw', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}) + + expect(stdin.setRawMode).toHaveBeenLastCalledWith(true) + stdin.press('\r') await promise - expect(mockStdin.unref).toBeCalled() - expect(mockStdin.setRawMode).toHaveBeenLastCalledWith(false) + + expect(stdin.setRawMode).toHaveBeenLastCalledWith(false) }) - test('rejects if sent ctrl+c', async () => { - let registeredListener: any - const mockStdin = { - setRawMode: vi.fn(), - once: (event: string, callback: any) => { - registeredListener = callback - }, - ref: vi.fn(), - unref: vi.fn(), - } as any + test('leaves raw mode on when stdin was already raw', async () => { + const stdin = new FakeStdin({isRaw: true}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}) - const promise = keypress(mockStdin, {skipTTYCheck: true}) + stdin.press('\r') + await promise - // create a buffer representing pressing ctrl+c - registeredListener(Buffer.from([3])) + expect(stdin.setRawMode).not.toHaveBeenCalled() + expect(stdin.isRaw).toBe(true) + }) - let rejected = false - try { - await promise - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (rejection: any) { - expect(rejection.type).toEqual(FatalErrorType.AbortSilent) - rejected = true - } - expect(rejected).toEqual(true) + test('stops listening to stdin and unrefs it once settled', async () => { + const stdin = new FakeStdin({isRaw: false}) + const promise = keypress(stdin.asStdin(), {skipTTYCheck: true}) + + expect(stdin.ref).toHaveBeenCalledTimes(1) + expect(stdin.listenerCount('data')).toBe(1) + stdin.press('\r') + await promise + + expect(stdin.listenerCount('data')).toBe(0) + expect(stdin.unref).toHaveBeenCalledTimes(1) }) }) diff --git a/packages/cli-kit/src/public/node/ui.tsx b/packages/cli-kit/src/public/node/ui.tsx index 6f65e8152e5..687e23bcf55 100644 --- a/packages/cli-kit/src/public/node/ui.tsx +++ b/packages/cli-kit/src/public/node/ui.tsx @@ -740,33 +740,64 @@ export async function renderDangerousConfirmationPrompt( }) } -/** Waits for any key to be pressed except Ctrl+C which will terminate the process. */ +interface KeypressOptions { + /** + * The keys that end the wait, for example `['\r']` for Enter. Other keys are ignored. + * Without this option, any key ends the wait. + */ + keys?: string[] +} -export const keypress = async (stdin = process.stdin, uiDebugOptions: UIDebugOptions = defaultUIDebugOptions) => { +/** + * Waits for a key to be pressed. Ctrl+C rejects with AbortSilentError, which terminates the process. + * + * Raw mode is set back to the state it had before the call. A command that keeps stdin raw for + * its own single-key shortcuts (for example `theme dev`) keeps them. + * + * Chunks are read as text: an active Ink renderer calls `stdin.setEncoding('utf8')`, and then + * stdin emits strings instead of Buffers. + * + * @param stdin - The stream to read from. + * @param uiDebugOptions - Debug options; `skipTTYCheck` allows a non-TTY stdin. + * @param options - The keys that end the wait. + * @returns A promise that resolves when an accepted key is pressed. + */ +export const keypress = async ( + stdin = process.stdin, + uiDebugOptions: UIDebugOptions = defaultUIDebugOptions, + {keys}: KeypressOptions = {}, +) => { throwInNonTTY({message: 'Press any key'}, uiDebugOptions) return runWithTimer('cmd_all_timing_prompts_ms')(() => { - return new Promise((resolve, reject) => { - const handler = (buffer: Buffer) => { - stdin.setRawMode(false) + const {promise, resolve, reject} = Promise.withResolvers() + const stdinWasRaw = stdin.isRaw === true - const bytes = Array.from(buffer) + const stopListening = () => { + stdin.off('data', handler) + if (!stdinWasRaw) stdin.setRawMode(false) + stdin.unref() + } - if (bytes.length && bytes[0] === 3) { - outputDebug('Canceled keypress, User pressed CTRL+C') - reject(new AbortSilentError()) - } - stdin.unref() + const handler = (chunk: Buffer | string) => { + const text = chunk.toString() + if (text.includes('\u0003')) { + stopListening() + outputDebug('Canceled keypress, User pressed CTRL+C') + reject(new AbortSilentError()) + } else if (!keys || [...text].some((character) => keys.includes(character))) { + stopListening() process.nextTick(resolve) } + } - stdin.setRawMode(true) - stdin.once('data', handler) + if (!stdinWasRaw) stdin.setRawMode(true) + stdin.on('data', handler) - // We want to indicate that we're still using stdin, so that the process - // doesn't exit early. - stdin.ref() - }) + // We want to indicate that we're still using stdin, so that the process + // doesn't exit early. + stdin.ref() + return promise }) }