From fd4a5dbd0f364c835c25ddef63ed333dedb1738c Mon Sep 17 00:00:00 2001 From: Suleiman Latrsh Date: Mon, 21 Sep 2026 14:37:13 -0400 Subject: [PATCH 1/3] Report unavailable shops and Admin API server errors instead of unknown CLI bugs Admin API version discovery wrapped every unclassified failure in a BugError, so a frozen shop (HTTP 402), a Shopify-side 5xx, and a cancelled request all surfaced as "Unknown error connecting to your store" and were filed as CLI bugs. Classify all three at the call site in cli-kit's fetchApiVersions, and wrap 5xx in @shopify/store's fetchPublicApiVersions so a raw ClientError no longer reaches the error reporter unwrapped. Patch authored by River (Pit Crew). Vault-Issue: 31609 Vault-Issue: 78419 Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/unavailable-shop-not-a-cli-bug.md | 6 + packages/cli-kit/src/private/node/api.ts | 27 ++++ .../cli-kit/src/public/node/api/admin.test.ts | 118 ++++++++++++++++++ packages/cli-kit/src/public/node/api/admin.ts | 34 ++++- .../store/execute/admin-transport.test.ts | 20 +++ .../services/store/execute/admin-transport.ts | 16 +++ 6 files changed, 220 insertions(+), 1 deletion(-) create mode 100644 .changeset/unavailable-shop-not-a-cli-bug.md 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..522a26ffbe6 100644 --- a/packages/cli-kit/src/private/node/api.ts +++ b/packages/cli-kit/src/private/node/api.ts @@ -138,6 +138,33 @@ export function isNetworkError(error: unknown): boolean { return false } +/** + * Lower-cased substrings the fetch implementations we run on use to signal that 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 signal says 'The operation + * was aborted'. Exported so tests assert against the same source of truth as production. + */ +export const ABORTED_FETCH_MESSAGE_FRAGMENTS = ['the user aborted a request', 'operation was aborted'] as const + +/** + * Checks if an error is an aborted request: the user cancelling the command, the host process + * cancelling it, or one of the CLI's own request timeouts firing. + * + * Deliberately kept out of `isTransientNetworkError`, which drives retry behaviour — a request that + * was cancelled on purpose must not be retried. The `name` check does not match cli-kit's own + * `AbortError`, because `FatalError` never assigns `name` (it stays 'Error'); it matches the + * `AbortError`/`DOMException` shapes that fetch itself throws. + * + * @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..6fc2e45625b 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,38 @@ export async function fetchApiVersions( ) } + // HTTP 402 Payment Required from the Admin API means the shop itself is unavailable: frozen, + // paused, or closed. That is an expected store-state condition the user can act on, not a CLI + // bug, so it must never 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 is a constant with no user-supplied input, so + // a server error here cannot have been caused by what the user typed. Note that 502/503/504 are + // already on `isExpectedApiError`'s list in node/error, but that list is only consulted for raw + // errors — it is never reached for an error 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 request that was cancelled — by the user, by the host process, or by one of the CLI's own + // timeouts — is neither a store-state failure nor a CLI bug, and has to stay distinguishable + // from both. Checked before `isNetworkError` so the message says the request was aborted rather + // than 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..3644ecd4664 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,22 @@ export async function fetchPublicApiVersions(input: { const classified = classifyAdminApiError(error, input.adminSession.storeFqdn) if (classified) throw classified + // Version discovery sends a constant query with no user input, so an Admin API 5xx here is a + // Shopify-side failure that cannot have been caused by the operation the user asked for. It is + // wrapped rather than rethrown because a raw `ClientError` escaping this function reaches the + // error reporter unwrapped (`shouldReportErrorAsUnexpected` only excuses 401/429/502/503/504) + // and is filed as an unexpected CLI bug, with the whole request echoed back at the user. + // + // Deliberately handled here and not in `classifyAdminApiError`, which `runAdminStoreGraphQLOperation` + // also calls: there a 5xx can carry GraphQL errors about the user's own query, and that + // function's `GraphQL operation failed.` branch has to stay reachable so those 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 } } From 94ebd89657796563329dd4bb1ca0a83635d1bc9b Mon Sep 17 00:00:00 2001 From: Suleiman Latrsh Date: Tue, 22 Sep 2026 10:04:15 -0400 Subject: [PATCH 2/3] Address review: trim comments, unexport constant, fix retry claim - Shorten the two JSDoc blocks on the aborted-fetch helpers. - Drop `export` from ABORTED_FETCH_MESSAGE_FRAGMENTS; nothing imports it. - Correct the claim that aborted requests are never retried. The CLI's own timeout message matches isTransientNetworkError, so those still retry. Only user-cancelled requests are excluded. Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli-kit/src/private/node/api.ts | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/cli-kit/src/private/node/api.ts b/packages/cli-kit/src/private/node/api.ts index 522a26ffbe6..969bbae3611 100644 --- a/packages/cli-kit/src/private/node/api.ts +++ b/packages/cli-kit/src/private/node/api.ts @@ -139,21 +139,22 @@ export function isNetworkError(error: unknown): boolean { } /** - * Lower-cased substrings the fetch implementations we run on use to signal that 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 signal says 'The operation - * was aborted'. Exported so tests assert against the same source of truth as production. + * 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'. */ -export const ABORTED_FETCH_MESSAGE_FRAGMENTS = ['the user aborted a request', 'operation was aborted'] as const +const ABORTED_FETCH_MESSAGE_FRAGMENTS = ['the user aborted a request', 'operation was aborted'] as const /** - * Checks if an error is an aborted request: the user cancelling the command, the host process + * 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. * - * Deliberately kept out of `isTransientNetworkError`, which drives retry behaviour — a request that - * was cancelled on purpose must not be retried. The `name` check does not match cli-kit's own - * `AbortError`, because `FatalError` never assigns `name` (it stays 'Error'); it matches the - * `AbortError`/`DOMException` shapes that fetch itself throws. + * 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. From c85b5de01c61e9d30e297b8a58e73190e7a1e1d6 Mon Sep 17 00:00:00 2001 From: Suleiman Latrsh Date: Tue, 22 Sep 2026 10:08:59 -0400 Subject: [PATCH 3/3] Apply the comment trimming to the rest of the PR Same treatment as the api.ts blocks: shorten the three inline comments in admin.ts and the nine-line block in admin-transport.ts. The reasoning that belongs in the PR description rather than the code is dropped. Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli-kit/src/public/node/api/admin.ts | 19 ++++++++----------- .../services/store/execute/admin-transport.ts | 12 ++++-------- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/packages/cli-kit/src/public/node/api/admin.ts b/packages/cli-kit/src/public/node/api/admin.ts index 6fc2e45625b..c70ad55e1f8 100644 --- a/packages/cli-kit/src/public/node/api/admin.ts +++ b/packages/cli-kit/src/public/node/api/admin.ts @@ -192,9 +192,8 @@ export async function fetchApiVersions( ) } - // HTTP 402 Payment Required from the Admin API means the shop itself is unavailable: frozen, - // paused, or closed. That is an expected store-state condition the user can act on, not a CLI - // bug, so it must never reach the BugError below. + // 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.`, @@ -202,10 +201,9 @@ export async function fetchApiVersions( ) } - // HTTP 5xx is a Shopify-side failure. This query is a constant with no user-supplied input, so - // a server error here cannot have been caused by what the user typed. Note that 502/503/504 are - // already on `isExpectedApiError`'s list in node/error, but that list is only consulted for raw - // errors — it is never reached for an error we have already wrapped in a BugError. + // 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}).`, @@ -213,10 +211,9 @@ export async function fetchApiVersions( ) } - // A request that was cancelled — by the user, by the host process, or by one of the CLI's own - // timeouts — is neither a store-state failure nor a CLI bug, and has to stay distinguishable - // from both. Checked before `isNetworkError` so the message says the request was aborted rather - // than blaming the user's connection. + // 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.`, 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 3644ecd4664..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,15 +59,11 @@ export async function fetchPublicApiVersions(input: { const classified = classifyAdminApiError(error, input.adminSession.storeFqdn) if (classified) throw classified - // Version discovery sends a constant query with no user input, so an Admin API 5xx here is a - // Shopify-side failure that cannot have been caused by the operation the user asked for. It is - // wrapped rather than rethrown because a raw `ClientError` escaping this function reaches the - // error reporter unwrapped (`shouldReportErrorAsUnexpected` only excuses 401/429/502/503/504) - // and is filed as an unexpected CLI bug, with the whole request echoed back at the user. + // 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. // - // Deliberately handled here and not in `classifyAdminApiError`, which `runAdminStoreGraphQLOperation` - // also calls: there a 5xx can carry GraphQL errors about the user's own query, and that - // function's `GraphQL operation failed.` branch has to stay reachable so those stay visible. + // 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}).`,