diff --git a/.changeset/unavailable-shop-not-a-cli-bug.md b/.changeset/unavailable-shop-not-a-cli-bug.md new file mode 100644 index 00000000000..8ee0ae90017 --- /dev/null +++ b/.changeset/unavailable-shop-not-a-cli-bug.md @@ -0,0 +1,6 @@ +--- +'@shopify/cli-kit': patch +'@shopify/store': patch +--- + +Stop reporting an unavailable shop, an Admin API server error, and a cancelled request as unknown CLI bugs during Admin API version discovery diff --git a/packages/cli-kit/src/private/node/api.ts b/packages/cli-kit/src/private/node/api.ts index 7fdaeb54640..969bbae3611 100644 --- a/packages/cli-kit/src/private/node/api.ts +++ b/packages/cli-kit/src/private/node/api.ts @@ -138,6 +138,34 @@ export function isNetworkError(error: unknown): boolean { return false } +/** + * Lower-cased substrings that mean a request was aborted rather than failing on its own. + * `node-fetch` says 'The user aborted a request.', undici says 'This operation was aborted', and + * cli-kit's own request timeout says 'The operation was aborted'. + */ +const ABORTED_FETCH_MESSAGE_FRAGMENTS = ['the user aborted a request', 'operation was aborted'] as const + +/** + * Checks if an error is an aborted request: a user cancelling the command, the host process + * cancelling it, or one of the CLI's own request timeouts firing. + * + * Not used by the retry logic, because a user-cancelled request must not be retried. + * `isTransientNetworkError` separately matches the CLI's own timeout message, so timeouts do + * still retry. + * + * The `name` check matches the `AbortError` shape that fetch throws, not cli-kit's own + * `AbortError`, which leaves `name` as 'Error'. + * + * @param error - Error to be checked. + * @returns A boolean indicating if the request was aborted. + */ +export function isAbortedFetchError(error: unknown): boolean { + if (!(error instanceof Error)) return false + if (error.name === 'AbortError') return true + const errorMessage = error.message.toLowerCase() + return ABORTED_FETCH_MESSAGE_FRAGMENTS.some((fragment) => errorMessage.includes(fragment)) +} + async function runRequestWithNetworkLevelRetry( requestOptions: RequestOptions, ): Promise { diff --git a/packages/cli-kit/src/public/node/api/admin.test.ts b/packages/cli-kit/src/public/node/api/admin.test.ts index 1a503448dfd..976f8d2bf6a 100644 --- a/packages/cli-kit/src/public/node/api/admin.test.ts +++ b/packages/cli-kit/src/public/node/api/admin.test.ts @@ -1,10 +1,12 @@ import * as admin from './admin.js' import {graphqlRequest, graphqlRequestDoc} from './graphql.js' import {AdminSession} from '../session.js' +import {AbortError, BugError, shouldReportErrorAsUnexpected} from '../error.js' import {buildHeaders} from '../../../private/node/api/headers.js' import * as http from '../http.js' import {defaultThemeKitAccessDomain} from '../../../private/node/constants.js' +import {ClientError} from 'graphql-request' import {test, vi, expect, describe} from 'vitest' vi.mock('./graphql.js') @@ -193,3 +195,119 @@ describe('admin-rest-api', () => { ) }) }) + +describe('fetchApiVersions error classification', () => { + // Mirrors `packages/cli-kit/src/public/node/error/index.test.ts`: a real `ClientError`, because + // the branches under test use `instanceof ClientError`. + function clientError(status: number, errors: unknown): ClientError { + return new ClientError( + {status, errors, headers: {}} as any, + { + query: 'query publicApiVersions { publicApiVersions { handle supported } }', + } as any, + ) + } + + test('reports a 402 Unavailable Shop as an expected store-state failure, not a CLI bug', async () => { + // Given + vi.mocked(graphqlRequestDoc).mockRejectedValue(clientError(402, 'Unavailable Shop')) + + // When + const error = await admin.fetchApiVersions(Session).catch((err: unknown) => err) + + // Then + expect(error).toBeInstanceOf(AbortError) + expect(error).not.toBeInstanceOf(BugError) + expect(shouldReportErrorAsUnexpected(error)).toBe(false) + expect((error as AbortError).message).toBe(`The store ${Session.storeFqdn} is currently unavailable.`) + expect(String((error as AbortError).tryMessage)).toContain('frozen, paused, or closed') + expect((error as AbortError).message).not.toContain('Unknown error') + }) + + test('reports an Admin API 5xx as an expected server-side failure, not a CLI bug', async () => { + // Given + vi.mocked(graphqlRequestDoc).mockRejectedValue(clientError(500, 'Internal Server Error')) + + // When + const error = await admin.fetchApiVersions(Session).catch((err: unknown) => err) + + // Then + expect(error).toBeInstanceOf(AbortError) + expect(error).not.toBeInstanceOf(BugError) + expect(shouldReportErrorAsUnexpected(error)).toBe(false) + expect((error as AbortError).message).toBe( + `The Admin API for ${Session.storeFqdn} returned a server error (HTTP 500).`, + ) + }) + + // The literal messages the runtimes emit, not the production constant: asserting against real + // observed wording keeps the test honest if someone edits the fragment list. + // 'The user aborted a request.' is node-fetch; 'This operation was aborted' is undici; + // 'The operation was aborted' is cli-kit's own request-timeout signal. + test.each(['The user aborted a request.', 'This operation was aborted', 'The operation was aborted'])( + 'keeps an aborted request (%j) distinguishable from a store-state failure', + async (fragment) => { + // Given + vi.mocked(graphqlRequestDoc).mockRejectedValue(new Error(fragment)) + + // When + const error = await admin.fetchApiVersions(Session).catch((err: unknown) => err) + + // Then + expect(error).toBeInstanceOf(AbortError) + expect(error).not.toBeInstanceOf(BugError) + expect(shouldReportErrorAsUnexpected(error)).toBe(false) + expect((error as AbortError).message).toBe(`Request to ${Session.storeFqdn} was aborted before it completed.`) + expect((error as AbortError).message).not.toContain('is currently unavailable') + }, + ) + + test('keeps an AbortError-named fetch rejection distinguishable too', async () => { + // Given + const aborted = new Error('aborted') + aborted.name = 'AbortError' + vi.mocked(graphqlRequestDoc).mockRejectedValue(aborted) + + // When + const error = await admin.fetchApiVersions(Session).catch((err: unknown) => err) + + // Then + expect(error).toBeInstanceOf(AbortError) + expect((error as AbortError).message).toBe(`Request to ${Session.storeFqdn} was aborted before it completed.`) + }) + + test('still reports a genuinely unknown failure as a CLI bug', async () => { + // Given + vi.mocked(graphqlRequestDoc).mockRejectedValue(new Error('something nobody has classified')) + + // When + const error = await admin.fetchApiVersions(Session).catch((err: unknown) => err) + + // Then + expect(error).toBeInstanceOf(BugError) + expect(shouldReportErrorAsUnexpected(error)).toBe(true) + expect((error as BugError).message).toContain('Unknown error connecting to your store') + }) + + test('leaves the existing 403 and 401 classifications alone', async () => { + // Given + vi.mocked(graphqlRequestDoc).mockRejectedValue(clientError(403, 'Forbidden')) + + // When + const forbidden = await admin.fetchApiVersions(Session).catch((err: unknown) => err) + + // Then + expect(forbidden).toBeInstanceOf(AbortError) + expect((forbidden as AbortError).message).toContain("Looks like you don't have access to this dev store") + + // Given + vi.mocked(graphqlRequestDoc).mockRejectedValue(clientError(401, 'Unauthorized')) + + // When + const unauthorized = await admin.fetchApiVersions(Session).catch((err: unknown) => err) + + // Then + expect(unauthorized).toBeInstanceOf(AbortError) + expect((unauthorized as AbortError).message).toContain('Error connecting to your store') + }) +}) diff --git a/packages/cli-kit/src/public/node/api/admin.ts b/packages/cli-kit/src/public/node/api/admin.ts index f496b9d533b..c70ad55e1f8 100644 --- a/packages/cli-kit/src/public/node/api/admin.ts +++ b/packages/cli-kit/src/public/node/api/admin.ts @@ -14,7 +14,7 @@ import { restRequestUrl, isThemeAccessSession, } from '../../../private/node/api/rest.js' -import {isNetworkError} from '../../../private/node/api.js' +import {isAbortedFetchError, isNetworkError} from '../../../private/node/api.js' import {RequestModeInput, shopifyFetch} from '../http.js' import {PublicApiVersions} from '../../../cli/api/graphql/admin/generated/public_api_versions.js' import {themeKitAccessDomain} from '../../../private/node/constants.js' @@ -192,6 +192,35 @@ export async function fetchApiVersions( ) } + // HTTP 402 means the shop is frozen, paused, or closed. That is a store state the user can + // fix, not a CLI bug, so it must not reach the BugError below. + if (error instanceof ClientError && error.response.status === 402) { + throw new AbortError( + `The store ${session.storeFqdn} is currently unavailable.`, + 'This usually means the store is frozen, paused, or closed. Check the store in the Shopify admin and try again once it is reactivated.', + ) + } + + // HTTP 5xx is a Shopify-side failure. This query takes no user input, so the user's command + // cannot have caused it. `isExpectedApiError` covers 502/503/504, but only for raw errors, so + // it never sees one we have already wrapped in a BugError. + if (error instanceof ClientError && error.response.status >= 500) { + throw new AbortError( + `The Admin API for ${session.storeFqdn} returned a server error (HTTP ${error.response.status}).`, + 'This is a problem on the Shopify side, not with your command. Wait a moment and try again.', + ) + } + + // A cancelled request is neither a store-state failure nor a CLI bug. Checked before + // `isNetworkError` so the message says the request was aborted instead of blaming the + // user's connection. + if (isAbortedFetchError(error)) { + throw new AbortError( + `Request to ${session.storeFqdn} was aborted before it completed.`, + 'The request was cancelled or timed out before the store responded. Try running the command again.', + ) + } + // Check for network-level errors (connection issues, timeouts, DNS failures, TLS/certificate errors, etc.) // All network errors should be treated as user-facing errors, not CLI bugs // Note: Some of these may have been retried already by lower-level retry logic diff --git a/packages/store/src/cli/services/store/execute/admin-transport.test.ts b/packages/store/src/cli/services/store/execute/admin-transport.test.ts index 65cb62f20c2..30aecd0d064 100644 --- a/packages/store/src/cli/services/store/execute/admin-transport.test.ts +++ b/packages/store/src/cli/services/store/execute/admin-transport.test.ts @@ -371,6 +371,26 @@ describe('fetchPublicApiVersions', () => { expect(clearStoredStoreAppSession).not.toHaveBeenCalled() }) + test('maps an Admin API 5xx to an AbortError instead of letting the raw client error escape', async () => { + vi.mocked(graphqlRequest).mockRejectedValue(makeClientErrorLike(500, 'Internal Server Error')) + + let captured: AbortError | undefined + await fetchPublicApiVersions({adminSession, session}).catch((error) => { + captured = error as AbortError + }) + + expect(captured).toBeInstanceOf(AbortError) + expect(captured).not.toBeInstanceOf(BugError) + // A raw `ClientError` reaching the reporter is filed as an unexpected CLI bug; an AbortError is not. + expect(captured?.message).toBe( + `Couldn't read the supported API versions for ${store}: the Admin API returned a server error (HTTP 500).`, + ) + expect(String((captured as unknown as {tryMessage?: string})?.tryMessage ?? '')).toContain( + 'This is a problem on the Shopify side', + ) + expect(clearStoredStoreAppSession).not.toHaveBeenCalled() + }) + test('rethrows unrelated errors', async () => { vi.mocked(graphqlRequest).mockRejectedValue(new Error('upstream exploded')) diff --git a/packages/store/src/cli/services/store/execute/admin-transport.ts b/packages/store/src/cli/services/store/execute/admin-transport.ts index 65ac1cc3572..86eabba21ce 100644 --- a/packages/store/src/cli/services/store/execute/admin-transport.ts +++ b/packages/store/src/cli/services/store/execute/admin-transport.ts @@ -59,6 +59,18 @@ export async function fetchPublicApiVersions(input: { const classified = classifyAdminApiError(error, input.adminSession.storeFqdn) if (classified) throw classified + // Version discovery takes no user input, so a 5xx here is a Shopify-side failure. Wrapped + // rather than rethrown: a raw `ClientError` is filed as an unexpected CLI bug. + // + // Not in `classifyAdminApiError`, which `runAdminStoreGraphQLOperation` also calls. There a + // 5xx can carry GraphQL errors about the user's own query, which must stay visible. + if (isGraphQLClientErrorLike(error) && typeof error.response.status === 'number' && error.response.status >= 500) { + throw new AbortError( + `Couldn't read the supported API versions for ${input.adminSession.storeFqdn}: the Admin API returned a server error (HTTP ${error.response.status}).`, + 'This is a problem on the Shopify side, not with your command. Wait a moment and run it again.', + ) + } + throw error } }