From e60fbf7f557ffc029ca7bf38425c1632fe8c26df Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Fri, 28 Aug 2026 14:35:48 +0200 Subject: [PATCH] Emit fatal errors as JSON --- .changeset/json-fatal-errors.md | 6 + docs/cli/error_handling.md | 29 ++ .../node/json-error.integration.test.ts | 46 +++ .../src/private/node/json-error.test.ts | 260 +++++++++++++++ .../cli-kit/src/private/node/json-error.ts | 181 +++++++++++ packages/cli-kit/src/public/node/error.ts | 285 +---------------- .../{error.test.ts => error/index.test.ts} | 59 +++- .../cli-kit/src/public/node/error/index.ts | 295 ++++++++++++++++++ .../src/public/node/error/schema.test.ts | 35 +++ .../cli-kit/src/public/node/error/schema.ts | 57 ++++ .../cli-kit/src/public/node/error/types.ts | 17 + packages/cli-kit/src/public/node/output.ts | 15 + packages/cli-kit/src/public/node/path.test.ts | 8 + packages/cli-kit/src/public/node/path.ts | 4 +- .../test/fixtures/cli-kit-source-loader.js | 10 + .../test/fixtures/json-error-exit-process.ts | 6 + .../test/fixtures/json-error-process.ts | 13 + packages/cli/src/bootstrap.ts | 16 +- packages/cli/src/index.ts | 14 +- ...uncaught-error-handler.integration.test.ts | 35 +++ .../cli/src/uncaught-error-handler.test.ts | 45 +++ packages/cli/src/uncaught-error-handler.ts | 33 ++ .../store/execute/admin-transport.test.ts | 33 +- .../services/store/execute/admin-transport.ts | 5 +- 24 files changed, 1190 insertions(+), 317 deletions(-) create mode 100644 .changeset/json-fatal-errors.md create mode 100644 packages/cli-kit/src/private/node/json-error.integration.test.ts create mode 100644 packages/cli-kit/src/private/node/json-error.test.ts create mode 100644 packages/cli-kit/src/private/node/json-error.ts rename packages/cli-kit/src/public/node/{error.test.ts => error/index.test.ts} (71%) create mode 100644 packages/cli-kit/src/public/node/error/index.ts create mode 100644 packages/cli-kit/src/public/node/error/schema.test.ts create mode 100644 packages/cli-kit/src/public/node/error/schema.ts create mode 100644 packages/cli-kit/src/public/node/error/types.ts create mode 100644 packages/cli-kit/test/fixtures/cli-kit-source-loader.js create mode 100644 packages/cli-kit/test/fixtures/json-error-exit-process.ts create mode 100644 packages/cli-kit/test/fixtures/json-error-process.ts create mode 100644 packages/cli/src/uncaught-error-handler.integration.test.ts create mode 100644 packages/cli/src/uncaught-error-handler.test.ts create mode 100644 packages/cli/src/uncaught-error-handler.ts diff --git a/.changeset/json-fatal-errors.md b/.changeset/json-fatal-errors.md new file mode 100644 index 00000000000..60bfb623b6e --- /dev/null +++ b/.changeset/json-fatal-errors.md @@ -0,0 +1,6 @@ +--- +'@shopify/cli-kit': minor +'@shopify/cli': minor +--- + +Emit machine-readable fatal errors when JSON output is active diff --git a/docs/cli/error_handling.md b/docs/cli/error_handling.md index 6b80794027f..073cb136867 100644 --- a/docs/cli/error_handling.md +++ b/docs/cli/error_handling.md @@ -138,6 +138,35 @@ The `FatalError` pattern will not work well in an architecture where app develop However, until then, we get a lot of leverage in the CLI from `FatalError`, so it can continue to exist as a high leverage counter-example to some of our general principles. +## Fatal errors in JSON output + +When `--json` or `-j` is active, a fatal error writes one document to stdout: + +```json +{"error":{"type":"abort","message":"Couldn't find an app","tryMessage":"Run shopify app config link","nextSteps":["Check that you're in the app directory"]}} +``` + +`type` is one of `abort`, `bug`, or `external`. `type` and `message` are always included. The regular error output's optional `tryMessage`, `nextSteps`, and `customSections` content is included as unstyled strings. Link URLs remain visible. Bug errors include `stack`; external errors include `command` and `args`. Other error properties are excluded. + +Callers can explicitly attach selected, JSON-serializable data to `error.details`. The renderer includes this field as +data, so consumers do not need to parse display strings. Do not attach a raw error, request, credentials, or other private +properties. For example, `store execute` exposes GraphQL errors at `error.details.errors`, including their error codes. + +```ts +const error = new AbortError('GraphQL operation failed.', JSON.stringify({errors}, null, 2)) +error.details = {errors} +throw error +``` + +The handler flushes stdout before returning to Oclif's exit path. If serialization fails, it emits a minimal JSON bug +error instead of a text banner. Stdout write failures are not retried as serialization failures. + +Absent optional fields are omitted. `nextSteps` is an array of strings. Each custom section has an optional `title` and a `body` containing either a string or a string matrix for tabular content. + +The process exit code remains the source of truth for success or failure. An `AbortSilentError` remains silent. Recoverable diagnostics and progress are written to stderr so stdout contains only the command result or fatal error document. + +This contract applies to execution-level failures. A failure represented by a command's result type belongs in that command's JSON result schema instead. + ## Report a result from a function There are scenarios where a function needs to inform the caller about the success or failure of the operation. For that, `@shopify/cli-kit` provides a result utility: diff --git a/packages/cli-kit/src/private/node/json-error.integration.test.ts b/packages/cli-kit/src/private/node/json-error.integration.test.ts new file mode 100644 index 00000000000..b452b242ec3 --- /dev/null +++ b/packages/cli-kit/src/private/node/json-error.integration.test.ts @@ -0,0 +1,46 @@ +import {execa} from 'execa' +import {describe, expect, test} from 'vitest' +import {fileURLToPath} from 'node:url' + +const fixturePath = fileURLToPath(new URL('../../../test/fixtures/json-error-process.ts', import.meta.url)) + +describe('JSON fatal error process output', () => { + test('flushes a large JSON document before Oclif exits', {timeout: 20000}, async () => { + const exitFixturePath = fileURLToPath(new URL('../../../test/fixtures/json-error-exit-process.ts', import.meta.url)) + const result = await execa(process.execPath, ['--loader', 'ts-node/esm', exitFixturePath, '--json'], { + env: {SHOPIFY_UNIT_TEST: 'false', FORCE_COLOR: '0', NODE_NO_WARNINGS: '1'}, + reject: false, + }) + + expect(result.exitCode, result.stderr).toBe(1) + expect(result.stderr).toBe('') + expect(JSON.parse(result.stdout)).toStrictEqual({ + error: {type: 'abort', message: 'x'.repeat(1024 * 1024)}, + }) + }) + + // Starting a fresh TypeScript subprocess needs extra startup headroom under loaded CI runners. + test( + 'writes one JSON document to stdout, diagnostics to stderr, and preserves the exit code', + {timeout: 20000}, + async () => { + const result = await execa(process.execPath, ['--loader', 'ts-node/esm', fixturePath, '--json'], { + env: {SHOPIFY_UNIT_TEST: 'false', FORCE_COLOR: '0', NODE_NO_WARNINGS: '1'}, + reject: false, + }) + + expect(result.exitCode, result.stderr).toBe(2) + expect(JSON.parse(result.stdout)).toStrictEqual({ + error: { + type: 'abort', + message: 'Expected failure', + tryMessage: 'Run shopify app dev again.', + nextSteps: ['Read the documentation (https://shopify.dev).'], + customSections: [{title: 'Details', body: 'The app could not be loaded.'}], + }, + }) + expect(result.stdout.trim().split('\n')).toHaveLength(1) + expect(result.stderr).toBe('Recoverable diagnostic') + }, + ) +}) diff --git a/packages/cli-kit/src/private/node/json-error.test.ts b/packages/cli-kit/src/private/node/json-error.test.ts new file mode 100644 index 00000000000..d1b61a5711d --- /dev/null +++ b/packages/cli-kit/src/private/node/json-error.test.ts @@ -0,0 +1,260 @@ +import {renderFatalErrorAsJson} from './json-error.js' +import {AbortError, AbortSilentError, BugError, ExternalError, FatalErrorType} from '../../public/node/error.js' +import {mockAndCaptureOutput} from '../../public/node/testing/output.js' +import * as output from '../../public/node/output.js' +import {afterEach, describe, expect, test, vi} from 'vitest' + +afterEach(() => { + mockAndCaptureOutput().clear() +}) + +function renderedDocument(error: Parameters[0]): unknown { + const output = mockAndCaptureOutput() + output.clear() + renderFatalErrorAsJson(error) + return JSON.parse(output.info()) +} + +describe('renderFatalErrorAsJson', () => { + test('includes only explicitly selected structured details', () => { + const details = {errors: [{message: 'Invalid field', extensions: {code: 'UNDEFINED_FIELD'}}]} + const error = Object.assign(new AbortError('GraphQL operation failed.'), { + details, + request: {authorization: 'secret'}, + accessToken: 'secret', + }) + + expect(renderedDocument(error)).toStrictEqual({ + error: {type: 'abort', message: 'GraphQL operation failed.', details}, + }) + }) + + test('falls back to a minimal JSON document when serialization fails', () => { + const error = Object.defineProperty(new AbortError('Expected failure'), 'formattedMessage', { + get: () => { + throw new Error('Could not read formatted message') + }, + }) + + expect(renderedDocument(error)).toStrictEqual({ + error: {type: 'bug', message: 'Failed to serialize the error as JSON.'}, + }) + }) + + test.each([ + ['BigInt', () => ({value: BigInt(1)})], + [ + 'circular reference', + () => { + const details: Record = {} + details.self = details + return details + }, + ], + ] as const)('falls back to JSON when details contain a %s', (_name, createDetails) => { + const error = new AbortError('Expected failure') + error.details = createDetails() + + expect(renderedDocument(error)).toStrictEqual({ + error: {type: 'bug', message: 'Failed to serialize the error as JSON.'}, + }) + }) + + test('does not retry stdout write failures as serialization failures', () => { + const error = Object.assign(new Error('write EPIPE'), {code: 'EPIPE'}) + const write = vi.spyOn(output, 'outputResult').mockImplementation(() => { + throw error + }) + + try { + expect(() => renderFatalErrorAsJson(new AbortError('Expected failure'))).toThrow(error) + expect(write).toHaveBeenCalledOnce() + } finally { + write.mockRestore() + } + }) + + test('does not inspect details on silent errors', () => { + const error = Object.defineProperty(new AbortSilentError(), 'details', { + get: () => { + throw new Error('Details must not be read') + }, + }) + + renderFatalErrorAsJson(error) + + expect(mockAndCaptureOutput().output()).toBe('') + }) + + test.each([ + ['abort', new AbortError('Expected failure'), {type: 'abort', message: 'Expected failure'}], + [ + 'bug', + new BugError('Unexpected failure'), + {type: 'bug', message: 'Unexpected failure', stack: expect.any(String)}, + ], + [ + 'external', + new ExternalError('External failure', 'npm', ['install']), + {type: 'external', message: 'External failure', command: 'npm', args: ['install']}, + ], + ])('renders a stable %s error', (_type, error, expectedError) => { + expect(renderedDocument(error)).toStrictEqual({error: expectedError}) + }) + + test('uses the rich message content and preserves link URLs', () => { + const error = new AbortError([ + 'Read', + {link: {label: 'the documentation', url: 'https://shopify.dev'}}, + {char: '.'}, + ]) + + expect(renderedDocument(error)).toStrictEqual({ + error: {type: 'abort', message: 'Read the documentation (https://shopify.dev).'}, + }) + }) + + test('includes a plain try message', () => { + expect(renderedDocument(new AbortError('Expected failure', 'Try again'))).toStrictEqual({ + error: {type: 'abort', message: 'Expected failure', tryMessage: 'Try again'}, + }) + }) + + test('flattens rich try message tokens to an unstyled string', () => { + const error = new AbortError('Expected failure', [ + '\u001B[31mRun\u001B[39m', + {command: 'shopify app dev'}, + {char: '.'}, + ]) + + expect(renderedDocument(error)).toStrictEqual({ + error: {type: 'abort', message: 'Expected failure', tryMessage: 'Run shopify app dev.'}, + }) + }) + + test('includes next steps as unstyled strings with visible link URLs', () => { + const error = new AbortError('Expected failure', null, [ + ['Read', {link: {label: 'the documentation', url: 'https://shopify.dev'}}, {char: '.'}], + '\u001B[31mTry again.\u001B[39m', + ]) + + expect(renderedDocument(error)).toStrictEqual({ + error: { + type: 'abort', + message: 'Expected failure', + nextSteps: ['Read the documentation (https://shopify.dev).', 'Try again.'], + }, + }) + }) + + test('includes custom text and tabular sections', () => { + const error = new AbortError('Expected failure', null, undefined, [ + { + title: '\u001B[31mExtension\u001B[39m', + body: [ + { + list: { + title: 'Validation errors', + items: ['Missing name', ['Read', {link: {label: 'the documentation', url: 'https://shopify.dev'}}]], + }, + }, + ], + }, + { + body: { + tabularData: [ + ['Name', {bold: 'Status'}], + ['checkout', '\u001B[31mFailed\u001B[39m'], + ], + }, + }, + ]) + + expect(renderedDocument(error)).toStrictEqual({ + error: { + type: 'abort', + message: 'Expected failure', + customSections: [ + { + title: 'Extension', + body: 'Validation errors: Missing name; Read the documentation (https://shopify.dev)', + }, + { + body: [ + ['Name', 'Status'], + ['checkout', 'Failed'], + ], + }, + ], + }, + }) + }) + + test('includes stacks only for bug errors', () => { + const bug = new BugError('Unexpected failure') + bug.stack = '\u001B[31mError: Unexpected failure\u001B[39m\n at example.ts:1:1' + const abort = new AbortError('Expected failure') + abort.stack = 'Error: Expected failure\n at example.ts:1:1' + + expect(renderedDocument(bug)).toStrictEqual({ + error: { + type: 'bug', + message: 'Unexpected failure', + stack: 'Error: Unexpected failure\n at example.ts:1:1', + }, + }) + expect(renderedDocument(abort)).toStrictEqual({ + error: {type: 'abort', message: 'Expected failure'}, + }) + }) + + test('omits empty optional collections', () => { + expect(renderedDocument(new AbortError('Expected failure', null, [], []))).toStrictEqual({ + error: {type: 'abort', message: 'Expected failure'}, + }) + }) + + test('omits malformed try messages without breaking the base error document', () => { + const error = Object.assign(new AbortError('Expected failure'), {tryMessage: {invalid: true}}) + + expect(renderedDocument(error)).toStrictEqual({ + error: {type: 'abort', message: 'Expected failure'}, + }) + }) + + test('does not render intentionally silent errors', () => { + const output = mockAndCaptureOutput() + output.clear() + + renderFatalErrorAsJson(new AbortSilentError()) + + expect(output.output()).toBe('') + }) + + test('includes external command context but not external stacks or arbitrary properties', () => { + const error = Object.assign(new ExternalError('Safe message', 'npm', ['install']), { + stack: 'external stack', + accessToken: 'secret', + request: {authorization: 'secret'}, + }) + + expect(renderedDocument(error)).toStrictEqual({ + error: {type: 'external', message: 'Safe message', command: 'npm', args: ['install']}, + }) + }) + + test('treats unknown fatal error types as bugs', () => { + expect(renderedDocument({type: 999, message: 'Future error'})).toStrictEqual({ + error: {type: 'bug', message: 'Future error'}, + }) + }) + + test('recognizes silent errors created by another cli-kit copy', () => { + const output = mockAndCaptureOutput() + output.clear() + + renderFatalErrorAsJson({type: FatalErrorType.AbortSilent, message: ''}) + + expect(output.output()).toBe('') + }) +}) diff --git a/packages/cli-kit/src/private/node/json-error.ts b/packages/cli-kit/src/private/node/json-error.ts new file mode 100644 index 00000000000..9945da03676 --- /dev/null +++ b/packages/cli-kit/src/private/node/json-error.ts @@ -0,0 +1,181 @@ +import {tokenItemToString, type Token, type TokenItem} from './ui/components/token-item.js' +import {FatalErrorType} from '../../public/node/error.js' +import {jsonErrorOutputSchema} from '../../public/node/error/schema.js' +import {outputResult, unstyled} from '../../public/node/output.js' +import type { + JsonError, + JsonErrorCustomSection, + JsonErrorDocument, + JsonErrorType, +} from '../../public/node/error/types.js' + +interface FatalErrorLike { + type?: number + message?: unknown + formattedMessage?: unknown + tryMessage?: unknown + nextSteps?: unknown + customSections?: unknown + stack?: unknown + command?: unknown + args?: unknown + details?: unknown +} + +interface ExternalCommand { + command: string + args: string[] +} + +function externalCommand(error: FatalErrorLike): ExternalCommand | undefined { + if (typeof error.command !== 'string' || !Array.isArray(error.args)) return + if (!error.args.every((arg) => typeof arg === 'string')) return + + return {command: error.command, args: error.args} +} + +function jsonErrorType(error: FatalErrorLike, external: ExternalCommand | undefined): JsonErrorType { + if (error.type === FatalErrorType.Abort) { + return external ? 'external' : 'abort' + } + return 'bug' +} + +function tokenToJsonString(token: Token): string { + if (typeof token === 'string') return token + + if ('link' in token) { + const {label, url} = token.link + return label && label !== url ? `${label} (${url})` : url + } + + if ('list' in token) { + const title = token.list.title ? tokenItemToJsonString(token.list.title).trim() : undefined + const items = token.list.items.map(tokenItemToJsonString).join('; ') + return title ? `${title}${items ? `: ${items}` : ''}` : items + } + + return tokenItemToString(token) +} + +function tokenItemToJsonString(token: TokenItem): string { + if (!Array.isArray(token)) return tokenToJsonString(token) + + return token + .map((item, index) => { + const value = tokenToJsonString(item) + const needsLeadingSpace = index !== 0 && !(typeof item !== 'string' && 'char' in item) + return needsLeadingSpace ? ` ${value}` : value + }) + .join('') +} + +function jsonTokenItem(token: unknown): string | undefined { + if (token === null || token === undefined) return + + try { + const message = tokenItemToJsonString(token as TokenItem) + return typeof message === 'string' ? unstyled(message) : undefined + } catch (error) { + if (error instanceof TypeError) return undefined + throw error + } +} + +function jsonTokenItems(items: unknown): string[] | undefined { + if (!Array.isArray(items)) return + + const renderedItems = items.map(jsonTokenItem).filter((item): item is string => item !== undefined) + return renderedItems.length > 0 ? renderedItems : undefined +} + +function jsonTable(data: unknown): string[][] | undefined { + if (!Array.isArray(data)) return + + return data + .filter((row): row is unknown[] => Array.isArray(row)) + .map((row) => row.map((cell) => jsonTokenItem(cell) ?? '')) +} + +function jsonCustomSection(section: unknown): JsonErrorCustomSection | undefined { + if (typeof section !== 'object' || section === null || !('body' in section)) return + + const title = 'title' in section && typeof section.title === 'string' ? unstyled(section.title) : undefined + const sectionBody = section.body + const body = + typeof sectionBody === 'object' && sectionBody !== null && 'tabularData' in sectionBody + ? jsonTable(sectionBody.tabularData) + : jsonTokenItem(sectionBody) + + if (body === undefined) return + return {...(title ? {title} : {}), body} +} + +function jsonCustomSections(sections: unknown): JsonErrorCustomSection[] | undefined { + if (!Array.isArray(sections)) return + + const renderedSections = sections + .map(jsonCustomSection) + .filter((section): section is JsonErrorCustomSection => section !== undefined) + return renderedSections.length > 0 ? renderedSections : undefined +} + +function jsonErrorDocument(error: FatalErrorLike): JsonErrorDocument | undefined { + if (error.type === FatalErrorType.AbortSilent) return + + const external = externalCommand(error) + const type = jsonErrorType(error, external) + const formattedMessage = jsonTokenItem(error.formattedMessage) + const message = formattedMessage ?? (typeof error.message === 'string' ? unstyled(error.message) : 'Unknown error') + const tryMessage = jsonTokenItem(error.tryMessage) + const nextSteps = jsonTokenItems(error.nextSteps) + const customSections = jsonCustomSections(error.customSections) + const details = error.details + + const commonFields = { + message, + ...(tryMessage === undefined ? {} : {tryMessage}), + ...(nextSteps === undefined ? {} : {nextSteps}), + ...(customSections === undefined ? {} : {customSections}), + ...(details === undefined ? {} : {details}), + } + + let jsonError: JsonError + if (type === 'bug') { + jsonError = { + type, + ...commonFields, + ...(typeof error.stack === 'string' ? {stack: unstyled(error.stack)} : {}), + } + } else if (type === 'external' && external) { + jsonError = {type, ...commonFields, ...external} + } else { + jsonError = {type: 'abort', ...commonFields} + } + + return {error: jsonError} +} + +/** + * Writes the public JSON representation of a fatal error to stdout. + * + * The allow-list mirrors the meaningful content of the regular fatal-error renderer. + * Arbitrary error properties remain private and are never copied to stdout. + * + * @param error - Fatal error to serialize. + */ +export function renderFatalErrorAsJson(error: FatalErrorLike): void { + let serializedDocument: string + try { + const document = jsonErrorDocument(error) + if (!document) return + serializedDocument = JSON.stringify(jsonErrorOutputSchema.validate(document)) + // Serialization must not replace the JSON contract with a text banner. + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + serializedDocument = JSON.stringify({error: {type: 'bug', message: 'Failed to serialize the error as JSON.'}}) + } + + // Keep write failures separate: retrying after a partial write could corrupt stdout. + outputResult(serializedDocument) +} diff --git a/packages/cli-kit/src/public/node/error.ts b/packages/cli-kit/src/public/node/error.ts index b5d7bba3c23..5351a569a64 100644 --- a/packages/cli-kit/src/public/node/error.ts +++ b/packages/cli-kit/src/public/node/error.ts @@ -1,284 +1 @@ -import {normalizePath} from './path.js' -import {OutputMessage, stringifyMessage, TokenizedString} from './output.js' -import {tokenItemToString, type InlineToken, type TokenItem} from '../../private/node/ui/components/token-item.js' -import {hasRateLimitCode} from '../../private/node/analytics/graphql-error-codes.js' - -import {Errors} from '@oclif/core' - -import type {AlertCustomSection} from './ui.js' - -export enum FatalErrorType { - Abort, - AbortSilent, - Bug, -} - -export class CancelExecution extends Error {} - -/** - * A fatal error represents an error shouldn't be rescued and that causes the execution to terminate. - * There shouldn't be code that catches fatal errors. - */ -export abstract class FatalError extends Error { - tryMessage: TokenItem | null - type: FatalErrorType - nextSteps?: TokenItem[] - formattedMessage?: TokenItem - customSections?: AlertCustomSection[] - skipOclifErrorHandling: boolean - /** - * Creates a new FatalError error. - * - * @param message - The error message. - * @param type - The type of fatal error. - * @param tryMessage - The message that recommends next steps to the user. - * You can pass a string a {@link TokenizedString} or a {@link TokenItem} - * if you need to style the message inside the error Banner component. - * @param nextSteps - Message to show as "next steps" with suggestions to solve the issue. - * @param customSections - Custom sections to show in the error banner. To be used if nextSteps is not enough. - */ - constructor( - message: TokenItem | OutputMessage, - type: FatalErrorType, - tryMessage: TokenItem | OutputMessage | null = null, - nextSteps?: TokenItem[], - customSections?: AlertCustomSection[], - ) { - const messageIsOutputMessage = typeof message === 'string' || 'value' in message - super(messageIsOutputMessage ? stringifyMessage(message) : tokenItemToString(message)) - - if (tryMessage) { - if (tryMessage instanceof TokenizedString) { - this.tryMessage = stringifyMessage(tryMessage) - } else { - this.tryMessage = tryMessage - } - } else { - this.tryMessage = null - } - - this.type = type - this.nextSteps = nextSteps - this.customSections = customSections - this.skipOclifErrorHandling = true - - if (!messageIsOutputMessage) { - this.formattedMessage = message - } - } -} - -/** - * An abort error is a fatal error that shouldn't be reported as a bug. - * Those usually represent unexpected scenarios that we can't handle and that usually require some action from the developer. - */ -export class AbortError extends FatalError { - constructor( - message: TokenItem | OutputMessage, - tryMessage: TokenItem | OutputMessage | null = null, - nextSteps?: TokenItem[], - customSections?: AlertCustomSection[], - ) { - super(message, FatalErrorType.Abort, tryMessage, nextSteps, customSections) - } -} - -/** - * An external error is similar to Abort but has extra command and args attributes. - * This is useful to represent errors coming from external commands, usually executed by execa. - */ -export class ExternalError extends FatalError { - command: string - args: string[] - - constructor( - message: OutputMessage, - command: string, - args: string[], - tryMessage: TokenItem | OutputMessage | null = null, - ) { - super(message, FatalErrorType.Abort, tryMessage) - this.command = command - this.args = args - } -} - -export class AbortSilentError extends FatalError { - constructor() { - super('', FatalErrorType.AbortSilent) - } -} - -/** - * A bug error is an error that represents a bug and therefore should be reported. - */ -export class BugError extends FatalError { - constructor(message: TokenItem | OutputMessage, tryMessage: TokenItem | OutputMessage | null = null) { - super(message, FatalErrorType.Bug, tryMessage) - } -} - -/** - * A function that handles errors that blow up in the CLI. - * - * @param error - Error to be handled. - * @returns A promise that resolves with the error passed. - */ -export async function handler(error: unknown): Promise { - let fatal: FatalError - if (isFatal(error)) { - fatal = error - } else if (typeof error === 'string') { - fatal = new BugError(error) - } else if (error instanceof Error) { - fatal = new BugError(error.message) - fatal.stack = error.stack - } else { - // errors can come in all shapes and sizes... - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const maybeError = error as any - fatal = new BugError(maybeError?.message ?? 'Unknown error') - if (maybeError?.stack) { - fatal.stack = maybeError?.stack - } - } - - const {renderFatalError} = await import('./ui.js') - renderFatalError(fatal) - return Promise.resolve(error) -} - -/** - * A function that maps an error to an Abort with the stack trace when coming from the CLI. - * - * @param error - Error to be mapped. - * @returns A promise that resolves with the new error object. - */ -export function errorMapper(error: unknown): Promise { - if (error instanceof Errors.CLIError) { - const mappedError = new AbortError(error.message) - mappedError.stack = error.stack - return Promise.resolve(mappedError) - } else { - return Promise.resolve(error) - } -} - -/** - * A function that checks if an error is a fatal one. - * - * @param error - Error to be checked. - * @returns A boolean indicating if the error is a fatal one. - */ -function isFatal(error: unknown): error is FatalError { - try { - return Object.prototype.hasOwnProperty.call(error, 'type') - // eslint-disable-next-line no-catch-all/no-catch-all - } catch { - return false - } -} - -/** - * A function that checks if an error should be reported as unexpected. - * - * @param error - Error to be checked. - * @returns A boolean indicating if the error should be reported as unexpected. - */ -export function shouldReportErrorAsUnexpected(error: unknown): boolean { - if (!isFatal(error)) { - // this means its not one of the CLI wrapped errors - if (error instanceof Error) { - // Raw API errors that slip through unwrapped (e.g. the handleErrors:false path) are expected - // environmental conditions, not CLI bugs. Treat them as expected so they don't pollute crash - // reporting. - if (isExpectedApiError(error)) { - return false - } - const message = error.message - return !errorMessageImpliesEnvironmentIssue(message) - } - return true - } - if (error.type === FatalErrorType.Bug) { - return true - } - return false -} - -/** - * Detects raw graphql-request `ClientError`s that are expected environmental conditions rather than - * CLI bugs. These reach the reporter as plain `Error`s (not `FatalError`s) and would otherwise be - * reported as unexpected. - * - * HTTP 401 (unauthenticated) is not "transient" in the retry sense — it means the user's session - * token is expired or invalid, a credential/environment condition (see issue #7891). Rate limiting - * (HTTP 429, or a `THROTTLED`/`429` GraphQL code on any error in the response) matches the shape - * detected by `errorsIncludeStatus429` in `private/node/api.ts`. Gateway errors (502/503/504) are - * also expected infrastructure failures. - * - * Scoped to the external `ClientError` shape only — importing the cli-kit `GraphQLClientError` - * wrapper here would create an `error.ts → headers.ts → error.ts` import cycle. - * - * Matched structurally rather than with `instanceof ClientError`, because importing the class - * pulls `graphql-request` — and through it `graphql`, `tr46` and `whatwg-url` — into the module - * graph of every command, for one type check on an error path. `ClientError` is the only error - * reaching here that carries both `response` and `request`; the cli-kit wrapper carries - * `statusCode` instead (see `private/node/api/headers.ts`). - * - * @param error - Error to be checked. - * @returns A boolean indicating if the error is a known expected API error. - */ -function isExpectedApiError(error: Error): boolean { - const candidate = error as Error & { - response?: {status?: number; errors?: unknown} - request?: unknown - } - if (typeof candidate.response !== 'object' || candidate.response === null || candidate.request === undefined) { - return false - } - const status = candidate.response.status - if (status === 401 || status === 429 || status === 502 || status === 503 || status === 504) { - return true - } - return hasRateLimitCode(candidate.response.errors) -} - -/** - * Stack traces usually have file:// - we strip that and also remove the Windows drive designation. - * - * @param filePath - Path to be cleaned. - * @returns The cleaned path. - */ -export function cleanSingleStackTracePath(filePath: string): string { - return normalizePath(filePath) - .replace('file:/', '/') - .replace(/^\/?[A-Z]:/, '') -} - -/** - * There are certain errors that we know are not due to a CLI bug, but are environmental/user error. - * - * @param message - The error message to check. - * @returns A boolean indicating if the error message implies an environment issue. - */ -function errorMessageImpliesEnvironmentIssue(message: string): boolean { - const environmentIssueMessages = [ - 'EPERM: operation not permitted, scandir', - 'EPERM: operation not permitted, rename', - 'EACCES: permission denied', - 'EPERM: operation not permitted, symlink', - 'This version of npm supports the following node versions', - 'EBUSY: resource busy or locked', - 'ENOTEMPTY: directory not empty', - 'getaddrinfo ENOTFOUND', - 'Client network socket disconnected before secure TLS connection was established', - 'spawn EPERM', - 'socket hang up', - 'The user aborted a request.', - 'write EPIPE', - 'Unsupported platform', - ] - const anyMatches = environmentIssueMessages.some((issueMessage) => message.includes(issueMessage)) - return anyMatches -} +export * from './error/index.js' diff --git a/packages/cli-kit/src/public/node/error.test.ts b/packages/cli-kit/src/public/node/error/index.test.ts similarity index 71% rename from packages/cli-kit/src/public/node/error.test.ts rename to packages/cli-kit/src/public/node/error/index.test.ts index 6698a5354d4..9958fe57271 100644 --- a/packages/cli-kit/src/public/node/error.test.ts +++ b/packages/cli-kit/src/public/node/error/index.test.ts @@ -1,14 +1,30 @@ -import {AbortError, BugError, handler, cleanSingleStackTracePath, shouldReportErrorAsUnexpected} from './error.js' -import {renderFatalError} from './ui.js' +import { + AbortError, + AbortSilentError, + BugError, + handler, + cleanSingleStackTracePath, + shouldReportErrorAsUnexpected, +} from '../error.js' +import {jsonOutputEnabled} from '../environment.js' +import {renderFatalError} from '../ui.js' +import {mockAndCaptureOutput} from '../testing/output.js' import {ClientError} from 'graphql-request' -import {describe, expect, test, vi} from 'vitest' +import {beforeEach, describe, expect, test, vi} from 'vitest' function clientError(status: number, code?: string): ClientError { const errors = code ? [{message: 'boom', extensions: {code}}] : undefined return new ClientError({status, errors, headers: {}} as any, {query: 'q'} as any) } -vi.mock('./ui.js') +vi.mock('../ui.js') +vi.mock('../environment.js') + +beforeEach(() => { + vi.mocked(jsonOutputEnabled).mockReturnValue(false) + vi.mocked(renderFatalError).mockClear() + mockAndCaptureOutput().clear() +}) describe('handler', () => { test('error output uses same input error instance when the error type is abort', async () => { @@ -47,6 +63,41 @@ describe('handler', () => { expect(renderFatalError).toHaveBeenCalledWith(expect.objectContaining({type: expect.any(Number)})) expect(unknownError).not.contains({type: expect.any(Number)}) }) + + test('renders one JSON document instead of a banner when JSON output is enabled', async () => { + const output = mockAndCaptureOutput() + vi.mocked(jsonOutputEnabled).mockReturnValue(true) + + await handler(new AbortError('Expected failure', 'Try again')) + + expect(JSON.parse(output.info())).toStrictEqual({ + error: {type: 'abort', message: 'Expected failure', tryMessage: 'Try again'}, + }) + expect(renderFatalError).not.toHaveBeenCalled() + }) + + test('keeps serialization failures in JSON mode', async () => { + const output = mockAndCaptureOutput() + const error = new AbortError('Expected failure') + error.details = {value: BigInt(1)} + vi.mocked(jsonOutputEnabled).mockReturnValue(true) + + await handler(error) + + expect(JSON.parse(output.info())).toStrictEqual({ + error: {type: 'bug', message: 'Failed to serialize the error as JSON.'}, + }) + expect(renderFatalError).not.toHaveBeenCalled() + }) + + test('keeps JSON silent aborts silent', async () => { + vi.mocked(jsonOutputEnabled).mockReturnValue(true) + + await handler(new AbortSilentError()) + + expect(mockAndCaptureOutput().output()).toBe('') + expect(renderFatalError).not.toHaveBeenCalled() + }) }) describe('stack file path helpers', () => { diff --git a/packages/cli-kit/src/public/node/error/index.ts b/packages/cli-kit/src/public/node/error/index.ts new file mode 100644 index 00000000000..841abf987f9 --- /dev/null +++ b/packages/cli-kit/src/public/node/error/index.ts @@ -0,0 +1,295 @@ +import {normalizePath} from '../path.js' +import {flushStdout, OutputMessage, stringifyMessage, TokenizedString} from '../output.js' +import {tokenItemToString, type InlineToken, type TokenItem} from '../../../private/node/ui/components/token-item.js' +import {hasRateLimitCode} from '../../../private/node/analytics/graphql-error-codes.js' + +import {Errors} from '@oclif/core' + +import type {AlertCustomSection} from '../ui.js' + +export enum FatalErrorType { + // These values are also read from errors created by other cli-kit copies. Do not renumber them. + Abort = 0, + AbortSilent = 1, + Bug = 2, +} + +export class CancelExecution extends Error {} + +/** + * A fatal error represents an error shouldn't be rescued and that causes the execution to terminate. + * There shouldn't be code that catches fatal errors. + */ +export abstract class FatalError extends Error { + tryMessage: TokenItem | null + type: FatalErrorType + nextSteps?: TokenItem[] + formattedMessage?: TokenItem + customSections?: AlertCustomSection[] + /** Selected JSON-serializable data to include in JSON errors. Never attach the raw error or request. */ + details?: unknown + skipOclifErrorHandling: boolean + /** + * Creates a new FatalError error. + * + * @param message - The error message. + * @param type - The type of fatal error. + * @param tryMessage - The message that recommends next steps to the user. + * You can pass a string a {@link TokenizedString} or a {@link TokenItem} + * if you need to style the message inside the error Banner component. + * @param nextSteps - Message to show as "next steps" with suggestions to solve the issue. + * @param customSections - Custom sections to show in the error banner. To be used if nextSteps is not enough. + */ + constructor( + message: TokenItem | OutputMessage, + type: FatalErrorType, + tryMessage: TokenItem | OutputMessage | null = null, + nextSteps?: TokenItem[], + customSections?: AlertCustomSection[], + ) { + const messageIsOutputMessage = typeof message === 'string' || 'value' in message + super(messageIsOutputMessage ? stringifyMessage(message) : tokenItemToString(message)) + + if (tryMessage) { + if (tryMessage instanceof TokenizedString) { + this.tryMessage = stringifyMessage(tryMessage) + } else { + this.tryMessage = tryMessage + } + } else { + this.tryMessage = null + } + + this.type = type + this.nextSteps = nextSteps + this.customSections = customSections + this.skipOclifErrorHandling = true + + if (!messageIsOutputMessage) { + this.formattedMessage = message + } + } +} + +/** + * An abort error is a fatal error that shouldn't be reported as a bug. + * Those usually represent unexpected scenarios that we can't handle and that usually require some action from the developer. + */ +export class AbortError extends FatalError { + constructor( + message: TokenItem | OutputMessage, + tryMessage: TokenItem | OutputMessage | null = null, + nextSteps?: TokenItem[], + customSections?: AlertCustomSection[], + ) { + super(message, FatalErrorType.Abort, tryMessage, nextSteps, customSections) + } +} + +/** + * An external error is similar to Abort but has extra command and args attributes. + * This is useful to represent errors coming from external commands, usually executed by execa. + */ +export class ExternalError extends FatalError { + command: string + args: string[] + + constructor( + message: OutputMessage, + command: string, + args: string[], + tryMessage: TokenItem | OutputMessage | null = null, + ) { + super(message, FatalErrorType.Abort, tryMessage) + this.command = command + this.args = args + } +} + +export class AbortSilentError extends FatalError { + constructor() { + super('', FatalErrorType.AbortSilent) + } +} + +/** + * A bug error is an error that represents a bug and therefore should be reported. + */ +export class BugError extends FatalError { + constructor(message: TokenItem | OutputMessage, tryMessage: TokenItem | OutputMessage | null = null) { + super(message, FatalErrorType.Bug, tryMessage) + } +} + +/** + * A function that handles errors that blow up in the CLI. + * + * @param error - Error to be handled. + * @returns A promise that resolves with the error passed. + */ +export async function handler(error: unknown): Promise { + let fatal: FatalError + if (isFatal(error)) { + fatal = error + } else if (typeof error === 'string') { + fatal = new BugError(error) + } else if (error instanceof Error) { + fatal = new BugError(error.message) + fatal.stack = error.stack + } else { + // errors can come in all shapes and sizes... + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const maybeError = error as any + fatal = new BugError(maybeError?.message ?? 'Unknown error') + if (maybeError?.stack) { + fatal.stack = maybeError?.stack + } + } + + const {jsonOutputEnabled} = await import('../environment.js') + if (jsonOutputEnabled()) { + const {renderFatalErrorAsJson} = await import('../../../private/node/json-error.js') + renderFatalErrorAsJson(fatal) + await flushStdout() + return error + } + + const {renderFatalError} = await import('../ui.js') + renderFatalError(fatal) + return Promise.resolve(error) +} + +/** + * A function that maps an error to an Abort with the stack trace when coming from the CLI. + * + * @param error - Error to be mapped. + * @returns A promise that resolves with the new error object. + */ +export function errorMapper(error: unknown): Promise { + if (error instanceof Errors.CLIError) { + const mappedError = new AbortError(error.message) + mappedError.stack = error.stack + return Promise.resolve(mappedError) + } else { + return Promise.resolve(error) + } +} + +/** + * A function that checks if an error is a fatal one. + * + * @param error - Error to be checked. + * @returns A boolean indicating if the error is a fatal one. + */ +function isFatal(error: unknown): error is FatalError { + try { + return Object.prototype.hasOwnProperty.call(error, 'type') + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + return false + } +} + +/** + * A function that checks if an error should be reported as unexpected. + * + * @param error - Error to be checked. + * @returns A boolean indicating if the error should be reported as unexpected. + */ +export function shouldReportErrorAsUnexpected(error: unknown): boolean { + if (!isFatal(error)) { + // this means its not one of the CLI wrapped errors + if (error instanceof Error) { + // Raw API errors that slip through unwrapped (e.g. the handleErrors:false path) are expected + // environmental conditions, not CLI bugs. Treat them as expected so they don't pollute crash + // reporting. + if (isExpectedApiError(error)) { + return false + } + const message = error.message + return !errorMessageImpliesEnvironmentIssue(message) + } + return true + } + if (error.type === FatalErrorType.Bug) { + return true + } + return false +} + +/** + * Detects raw graphql-request `ClientError`s that are expected environmental conditions rather than + * CLI bugs. These reach the reporter as plain `Error`s (not `FatalError`s) and would otherwise be + * reported as unexpected. + * + * HTTP 401 (unauthenticated) is not "transient" in the retry sense — it means the user's session + * token is expired or invalid, a credential/environment condition (see issue #7891). Rate limiting + * (HTTP 429, or a `THROTTLED`/`429` GraphQL code on any error in the response) matches the shape + * detected by `errorsIncludeStatus429` in `private/node/api.ts`. Gateway errors (502/503/504) are + * also expected infrastructure failures. + * + * Scoped to the external `ClientError` shape only — importing the cli-kit `GraphQLClientError` + * wrapper here would create an `error.ts → headers.ts → error.ts` import cycle. + * + * Matched structurally rather than with `instanceof ClientError`, because importing the class + * pulls `graphql-request` — and through it `graphql`, `tr46` and `whatwg-url` — into the module + * graph of every command, for one type check on an error path. `ClientError` is the only error + * reaching here that carries both `response` and `request`; the cli-kit wrapper carries + * `statusCode` instead (see `private/node/api/headers.ts`). + * + * @param error - Error to be checked. + * @returns A boolean indicating if the error is a known expected API error. + */ +function isExpectedApiError(error: Error): boolean { + const candidate = error as Error & { + response?: {status?: number; errors?: unknown} + request?: unknown + } + if (typeof candidate.response !== 'object' || candidate.response === null || candidate.request === undefined) { + return false + } + const status = candidate.response.status + if (status === 401 || status === 429 || status === 502 || status === 503 || status === 504) { + return true + } + return hasRateLimitCode(candidate.response.errors) +} + +/** + * Stack traces usually have file:// - we strip that and also remove the Windows drive designation. + * + * @param filePath - Path to be cleaned. + * @returns The cleaned path. + */ +export function cleanSingleStackTracePath(filePath: string): string { + return normalizePath(filePath) + .replace('file:/', '/') + .replace(/^\/?[A-Z]:/, '') +} + +/** + * There are certain errors that we know are not due to a CLI bug, but are environmental/user error. + * + * @param message - The error message to check. + * @returns A boolean indicating if the error message implies an environment issue. + */ +function errorMessageImpliesEnvironmentIssue(message: string): boolean { + const environmentIssueMessages = [ + 'EPERM: operation not permitted, scandir', + 'EPERM: operation not permitted, rename', + 'EACCES: permission denied', + 'EPERM: operation not permitted, symlink', + 'This version of npm supports the following node versions', + 'EBUSY: resource busy or locked', + 'ENOTEMPTY: directory not empty', + 'getaddrinfo ENOTFOUND', + 'Client network socket disconnected before secure TLS connection was established', + 'spawn EPERM', + 'socket hang up', + 'The user aborted a request.', + 'write EPIPE', + 'Unsupported platform', + ] + const anyMatches = environmentIssueMessages.some((issueMessage) => message.includes(issueMessage)) + return anyMatches +} diff --git a/packages/cli-kit/src/public/node/error/schema.test.ts b/packages/cli-kit/src/public/node/error/schema.test.ts new file mode 100644 index 00000000000..cba667f7dde --- /dev/null +++ b/packages/cli-kit/src/public/node/error/schema.test.ts @@ -0,0 +1,35 @@ +import {jsonErrorOutputSchema} from './schema.js' +import {describe, expect, test} from 'vitest' + +describe('JSON error output schema', () => { + test('documents and validates every fatal JSON error type', () => { + expect(jsonErrorOutputSchema.typescript).toContain( + 'type JsonError = JsonAbortError | JsonBugError | JsonExternalError', + ) + expect(jsonErrorOutputSchema.typescript).toContain('interface JsonAbortError') + expect(jsonErrorOutputSchema.typescript).toContain('interface JsonBugError') + expect(jsonErrorOutputSchema.typescript).toContain('interface JsonExternalError') + + expect(jsonErrorOutputSchema.validate({error: {type: 'abort', message: 'Expected failure'}})).toEqual({ + error: {type: 'abort', message: 'Expected failure'}, + }) + }) + + test('rejects an invalid fatal JSON error', () => { + expect(() => jsonErrorOutputSchema.validate({error: {type: 'external', message: 'Failed'}})).toThrow() + }) + + test.each([ + {type: 'abort' as const}, + {type: 'bug' as const}, + {type: 'external' as const, command: 'npm', args: ['install']}, + ])('supports selected structured details on $type errors', (variant) => { + const document = { + error: {...variant, message: 'Failed', details: {errors: [{message: 'Invalid field', code: 'UNDEFINED_FIELD'}]}}, + } + + expect(jsonErrorOutputSchema.validate(document)).toEqual(document) + expect(JSON.parse(jsonErrorOutputSchema.encode(document))).toEqual(document) + expect(jsonErrorOutputSchema.typescript).toContain('details?: unknown') + }) +}) diff --git a/packages/cli-kit/src/public/node/error/schema.ts b/packages/cli-kit/src/public/node/error/schema.ts new file mode 100644 index 00000000000..4ea4396b493 --- /dev/null +++ b/packages/cli-kit/src/public/node/error/schema.ts @@ -0,0 +1,57 @@ +import {defineJsonOutputSchema} from '../json-output-schema.js' +import {zod} from '../schema.js' + +export const JsonErrorCustomSectionSchema = zod + .object({ + title: zod.string().optional(), + body: zod.union([zod.string(), zod.array(zod.array(zod.string()))]), + }) + .strict() + +const commonJsonErrorShape = { + message: zod.string(), + tryMessage: zod.string().optional(), + nextSteps: zod.array(zod.string()).optional(), + customSections: zod.array(JsonErrorCustomSectionSchema).optional(), + details: zod.unknown().optional(), +} + +export const JsonAbortErrorSchema = zod + .object({ + type: zod.literal('abort'), + ...commonJsonErrorShape, + }) + .strict() + +export const JsonBugErrorSchema = zod + .object({ + type: zod.literal('bug'), + ...commonJsonErrorShape, + stack: zod.string().optional(), + }) + .strict() + +export const JsonExternalErrorSchema = zod + .object({ + type: zod.literal('external'), + ...commonJsonErrorShape, + command: zod.string(), + args: zod.array(zod.string()), + }) + .strict() + +export const JsonErrorSchema = zod.union([JsonAbortErrorSchema, JsonBugErrorSchema, JsonExternalErrorSchema]) + +const JsonErrorDocumentSchema = zod.object({error: JsonErrorSchema}).strict() + +export const jsonErrorOutputSchema = defineJsonOutputSchema({ + name: 'JsonErrorDocument', + schema: JsonErrorDocumentSchema, + definitions: { + JsonError: JsonErrorSchema, + JsonErrorCustomSection: JsonErrorCustomSectionSchema, + JsonAbortError: JsonAbortErrorSchema, + JsonBugError: JsonBugErrorSchema, + JsonExternalError: JsonExternalErrorSchema, + }, +}) diff --git a/packages/cli-kit/src/public/node/error/types.ts b/packages/cli-kit/src/public/node/error/types.ts new file mode 100644 index 00000000000..f36091529cc --- /dev/null +++ b/packages/cli-kit/src/public/node/error/types.ts @@ -0,0 +1,17 @@ +import type { + JsonAbortErrorSchema, + JsonBugErrorSchema, + JsonErrorCustomSectionSchema, + JsonErrorSchema, + JsonExternalErrorSchema, + jsonErrorOutputSchema, +} from './schema.js' +import type {z} from 'zod' + +export type JsonErrorCustomSection = z.infer +export type JsonAbortError = z.infer +export type JsonBugError = z.infer +export type JsonExternalError = z.infer +export type JsonError = z.infer +export type JsonErrorType = JsonError['type'] +export type JsonErrorDocument = z.infer diff --git a/packages/cli-kit/src/public/node/output.ts b/packages/cli-kit/src/public/node/output.ts index 8c6e7f27630..5ebb758c8fa 100644 --- a/packages/cli-kit/src/public/node/output.ts +++ b/packages/cli-kit/src/public/node/output.ts @@ -256,6 +256,21 @@ export function outputResult(content: OutputMessage): void { output(content, 'info', consoleLog) } +/** + * Waits for queued stdout writes to reach their destination before the process exits. + * + * @returns A promise that resolves when stdout has flushed. + */ +export async function flushStdout(): Promise { + // An empty write completes after the preceding writes, including those queued for a pipe. + await new Promise((resolve, reject) => { + process.stdout.write('', (error) => { + if (error) reject(error) + else resolve() + }) + }) +} + /** * Logs information at the info level. * Info messages don't get additional formatting. diff --git a/packages/cli-kit/src/public/node/path.test.ts b/packages/cli-kit/src/public/node/path.test.ts index 422e3649155..0a0a520a030 100644 --- a/packages/cli-kit/src/public/node/path.test.ts +++ b/packages/cli-kit/src/public/node/path.test.ts @@ -131,6 +131,14 @@ describe('sniffForJson', () => { test('returns false if neither is present', () => { expect(sniffForJson(['node', 'script.js', '--other-flag'])).toBe(false) }) + + test.each(['--json', '-j'])('returns false if %s is a passthrough argument', (jsonFlag) => { + expect(sniffForJson(['node', 'script.js', '--', jsonFlag])).toBe(false) + }) + + test('does not treat clustered short flags as JSON output', () => { + expect(sniffForJson(['node', 'script.js', '-vj'])).toBe(false) + }) }) describe('sanitizeRelativePath', () => { diff --git a/packages/cli-kit/src/public/node/path.ts b/packages/cli-kit/src/public/node/path.ts index f3721780110..17af3ed7366 100644 --- a/packages/cli-kit/src/public/node/path.ts +++ b/packages/cli-kit/src/public/node/path.ts @@ -203,7 +203,9 @@ export function sniffForPath(argv = process.argv): string | undefined { * @returns Whether the `--json` or `-j` flag is present in the arguments. */ export function sniffForJson(argv = process.argv): boolean { - return argv.includes('--json') || argv.includes('-j') + const passthroughIndex = argv.indexOf('--') + const commandArguments = passthroughIndex === -1 ? argv : argv.slice(0, passthroughIndex) + return commandArguments.includes('--json') || commandArguments.includes('-j') } /** diff --git a/packages/cli-kit/test/fixtures/cli-kit-source-loader.js b/packages/cli-kit/test/fixtures/cli-kit-source-loader.js new file mode 100644 index 00000000000..62d46e865af --- /dev/null +++ b/packages/cli-kit/test/fixtures/cli-kit-source-loader.js @@ -0,0 +1,10 @@ +// Unit-test subprocesses must use source files because CI does not build packages before testing. +export function resolve(specifier, context, nextResolve) { + if (specifier.startsWith('@shopify/cli-kit/')) { + const modulePath = specifier.slice('@shopify/cli-kit/'.length) + const sourceUrl = new URL(`../../src/public/${modulePath}.js`, import.meta.url) + return nextResolve(sourceUrl.href, context) + } + + return nextResolve(specifier, context) +} diff --git a/packages/cli-kit/test/fixtures/json-error-exit-process.ts b/packages/cli-kit/test/fixtures/json-error-exit-process.ts new file mode 100644 index 00000000000..e658c36c58e --- /dev/null +++ b/packages/cli-kit/test/fixtures/json-error-exit-process.ts @@ -0,0 +1,6 @@ +import {AbortError, handler} from '../../src/public/node/error.js' +import {Errors} from '@oclif/core' + +const error = new AbortError('x'.repeat(1024 * 1024)) +await handler(error) +await Errors.handle(error) diff --git a/packages/cli-kit/test/fixtures/json-error-process.ts b/packages/cli-kit/test/fixtures/json-error-process.ts new file mode 100644 index 00000000000..bd683e5ccce --- /dev/null +++ b/packages/cli-kit/test/fixtures/json-error-process.ts @@ -0,0 +1,13 @@ +import {AbortError, handler} from '../../src/public/node/error.js' +import {outputInfo} from '../../src/public/node/output.js' + +outputInfo('Recoverable diagnostic') +await handler( + new AbortError( + 'Expected failure', + ['Run', {command: 'shopify app dev'}, 'again.'], + [['Read', {link: {label: 'the documentation', url: 'https://shopify.dev'}}, {char: '.'}]], + [{title: 'Details', body: 'The app could not be loaded.'}], + ), +) +process.exitCode = 2 diff --git a/packages/cli/src/bootstrap.ts b/packages/cli/src/bootstrap.ts index 7e99f28509e..0eddb65e982 100644 --- a/packages/cli/src/bootstrap.ts +++ b/packages/cli/src/bootstrap.ts @@ -6,11 +6,10 @@ * Commands are loaded lazily by oclif from the manifest + index.ts only when needed. */ import {loadCommand} from './command-registry.js' +import {renderUncaughtError} from './uncaught-error-handler.js' import {createGlobalProxyAgent} from 'global-agent' import {runCLI} from '@shopify/cli-kit/node/cli' -import fs from 'fs' - // Setup global support for environment variable based proxy configuration. createGlobalProxyAgent({ environmentVariableNamespace: 'SHOPIFY_', @@ -26,18 +25,7 @@ createGlobalProxyAgent({ // makes sure that there are no lingering tunnel processes. // eslint-disable-next-line @typescript-eslint/no-misused-promises process.on('uncaughtException', async (err) => { - try { - const {FatalError} = await import('@shopify/cli-kit/node/error') - if (err instanceof FatalError) { - const {renderFatalError} = await import('@shopify/cli-kit/node/ui') - renderFatalError(err) - } else { - fs.writeSync(process.stderr.fd, `${err.stack ?? err.message ?? err}\n`) - } - // eslint-disable-next-line no-catch-all/no-catch-all - } catch { - fs.writeSync(process.stderr.fd, `${err.stack ?? err.message ?? err}\n`) - } + await renderUncaughtError(err) process.exit(1) }) const signals = ['SIGINT', 'SIGTERM', 'SIGQUIT'] diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 027585eac79..a7a087bd4ce 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -15,6 +15,7 @@ import DocFetch from './cli/commands/doc/fetch.js' import DocSearch from './cli/commands/doc/search.js' import DocsGenerate from './cli/commands/docs/generate.js' import HelpCommand from './cli/commands/help.js' +import {renderUncaughtError} from './uncaught-error-handler.js' import List from './cli/commands/notifications/list.js' import Generate from './cli/commands/notifications/generate.js' import ClearCache from './cli/commands/cache/clear.js' @@ -30,10 +31,6 @@ import {commands as PluginCommandsCommands} from '@oclif/plugin-commands' import {commands as PluginPluginsCommands} from '@oclif/plugin-plugins' import {DidYouMeanCommands} from '@shopify/plugin-did-you-mean' import {runCLI} from '@shopify/cli-kit/node/cli' -import {renderFatalError} from '@shopify/cli-kit/node/ui' -import {FatalError} from '@shopify/cli-kit/node/error' - -import fs from 'fs' export {DidYouMeanHook} from '@shopify/plugin-did-you-mean' export {default as TunnelStartHook} from '@shopify/plugin-cloudflare/hooks/tunnel' @@ -57,12 +54,9 @@ createGlobalProxyAgent({ // not be called. The tunnel plugin is an example of that. Here we make sure to print // the error stack and manually call exit so that the cleanup code is called. This // makes sure that there are no lingering tunnel processes. -process.on('uncaughtException', (err) => { - if (err instanceof FatalError) { - renderFatalError(err) - } else { - fs.writeSync(process.stderr.fd, `${err.stack ?? err.message ?? err}\n`) - } +// eslint-disable-next-line @typescript-eslint/no-misused-promises +process.on('uncaughtException', async (err) => { + await renderUncaughtError(err) process.exit(1) }) const signals = ['SIGINT', 'SIGTERM', 'SIGQUIT'] diff --git a/packages/cli/src/uncaught-error-handler.integration.test.ts b/packages/cli/src/uncaught-error-handler.integration.test.ts new file mode 100644 index 00000000000..4ec12dc95b6 --- /dev/null +++ b/packages/cli/src/uncaught-error-handler.integration.test.ts @@ -0,0 +1,35 @@ +import {describe, expect, test} from 'vitest' +import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system' + +const errorMessageLength = 1024 * 1024 +const handlerUrl = new URL('./uncaught-error-handler.ts', import.meta.url).href +const sourceLoaderUrl = new URL('../../cli-kit/test/fixtures/cli-kit-source-loader.js', import.meta.url).href + +describe('uncaught JSON error process output', () => { + test('flushes a JSON error to piped stdout before the process exits', {timeout: 20000}, async () => { + const script = ` + const {renderUncaughtError} = await import(${JSON.stringify(handlerUrl)}) + await renderUncaughtError({type: 0, message: 'x'.repeat(${errorMessageLength})}) + process.exit(1) + ` + const result = await captureOutputWithExitCode( + process.execPath, + ['--loader', 'ts-node/esm', '--loader', sourceLoaderUrl, '--input-type=module', '--eval', script], + { + env: { + ...process.env, + FORCE_COLOR: '0', + NODE_NO_WARNINGS: '1', + SHOPIFY_UNIT_TEST: 'false', + SHOPIFY_FLAG_JSON: '1', + }, + }, + ) + + expect(result.exitCode, result.stderr).toBe(1) + expect(result.stderr).toBe('') + expect(JSON.parse(result.stdout)).toStrictEqual({ + error: {type: 'abort', message: 'x'.repeat(errorMessageLength)}, + }) + }) +}) diff --git a/packages/cli/src/uncaught-error-handler.test.ts b/packages/cli/src/uncaught-error-handler.test.ts new file mode 100644 index 00000000000..fa3eca21335 --- /dev/null +++ b/packages/cli/src/uncaught-error-handler.test.ts @@ -0,0 +1,45 @@ +import {renderUncaughtError} from './uncaught-error-handler.js' +import {beforeEach, describe, expect, test, vi} from 'vitest' + +const mocks = vi.hoisted(() => { + class FatalError extends Error {} + + return { + FatalError, + handler: vi.fn(), + jsonOutputEnabled: vi.fn(), + renderFatalError: vi.fn(), + } +}) + +vi.mock('@shopify/cli-kit/node/environment', () => ({jsonOutputEnabled: mocks.jsonOutputEnabled})) +vi.mock('@shopify/cli-kit/node/error', () => ({FatalError: mocks.FatalError, handler: mocks.handler})) +vi.mock('@shopify/cli-kit/node/ui', () => ({renderFatalError: mocks.renderFatalError})) + +beforeEach(() => { + mocks.handler.mockReset() + mocks.jsonOutputEnabled.mockReset() + mocks.renderFatalError.mockReset() +}) + +describe('renderUncaughtError', () => { + test('uses the shared error handler for JSON output', async () => { + const error = new Error('Unexpected failure') + mocks.jsonOutputEnabled.mockReturnValue(true) + + await renderUncaughtError(error) + + expect(mocks.handler).toHaveBeenCalledWith(error) + expect(mocks.renderFatalError).not.toHaveBeenCalled() + }) + + test('preserves fatal error banners outside JSON output', async () => { + const error = new mocks.FatalError('Expected failure') + mocks.jsonOutputEnabled.mockReturnValue(false) + + await renderUncaughtError(error) + + expect(mocks.renderFatalError).toHaveBeenCalledWith(error) + expect(mocks.handler).not.toHaveBeenCalled() + }) +}) diff --git a/packages/cli/src/uncaught-error-handler.ts b/packages/cli/src/uncaught-error-handler.ts new file mode 100644 index 00000000000..4bb9c03e32d --- /dev/null +++ b/packages/cli/src/uncaught-error-handler.ts @@ -0,0 +1,33 @@ +import fs from 'fs' + +function writeRawError(error: unknown): void { + const message = error instanceof Error ? (error.stack ?? error.message) : String(error) + fs.writeSync(process.stderr.fd, `${message}\n`) +} + +/** + * Renders an exception raised outside oclif's command lifecycle. + * + * @param error - Uncaught exception to render. + */ +export async function renderUncaughtError(error: unknown): Promise { + try { + const {jsonOutputEnabled} = await import('@shopify/cli-kit/node/environment') + if (jsonOutputEnabled()) { + const {handler} = await import('@shopify/cli-kit/node/error') + await handler(error) + return + } + + const {FatalError} = await import('@shopify/cli-kit/node/error') + if (error instanceof FatalError) { + const {renderFatalError} = await import('@shopify/cli-kit/node/ui') + renderFatalError(error) + } else { + writeRawError(error) + } + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + writeRawError(error) + } +} 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 c914c978cde..65cb62f20c2 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 @@ -9,7 +9,8 @@ import {clearStoredStoreAppSession} from '@shopify/cli-kit/node/store-auth-sessi import {beforeEach, describe, expect, test, vi} from 'vitest' import {adminUrl} from '@shopify/cli-kit/node/api/admin' import {graphqlRequest} from '@shopify/cli-kit/node/api/graphql' -import {AbortError, BugError} from '@shopify/cli-kit/node/error' +import {AbortError, BugError, handler} from '@shopify/cli-kit/node/error' +import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' import {renderSingleTask} from '@shopify/cli-kit/node/ui' vi.mock('@shopify/cli-kit/node/store-auth-session') @@ -137,10 +138,36 @@ describe('runAdminStoreGraphQLOperation', () => { }) test('throws a GraphQL operation error when errors are returned', async () => { - vi.mocked(graphqlRequest).mockRejectedValue({response: {errors: [{message: 'Field does not exist'}]}}) + const errors = [{message: 'Field does not exist', extensions: {code: 'UNDEFINED_FIELD'}, path: ['nope']}] + vi.mocked(graphqlRequest).mockRejectedValue({response: {errors}}) const request = await prepareStoreExecuteRequest({query: 'query { nope }'}) - await expect(runAdminStoreGraphQLOperation({context, request})).rejects.toThrow('GraphQL operation failed.') + const error: unknown = await runAdminStoreGraphQLOperation({context, request}).catch((error: unknown) => error) + expect(error).toBeInstanceOf(AbortError) + expect(error).toMatchObject({ + message: 'GraphQL operation failed.', + tryMessage: JSON.stringify({errors}, null, 2), + details: {errors}, + }) + + const output = mockAndCaptureOutput() + output.clear() + vi.stubEnv('SHOPIFY_FLAG_JSON', '1') + try { + await handler(error) + + expect(JSON.parse(output.info())).toStrictEqual({ + error: { + type: 'abort', + message: 'GraphQL operation failed.', + tryMessage: JSON.stringify({errors}, null, 2), + details: {errors}, + }, + }) + } finally { + vi.unstubAllEnvs() + output.clear() + } }) test('maps a 402 ClientError to a store-unavailable AbortError even when the response also carries `errors`', async () => { 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 09aafa7932d..c13c569f9ce 100644 --- a/packages/store/src/cli/services/store/execute/admin-transport.ts +++ b/packages/store/src/cli/services/store/execute/admin-transport.ts @@ -91,7 +91,10 @@ export async function runAdminStoreGraphQLOperation(input: { if (classified) throw classified if (isGraphQLClientErrorLike(error) && error.response.errors) { - throw new AbortError('GraphQL operation failed.', JSON.stringify({errors: error.response.errors}, null, 2)) + const details = {errors: error.response.errors} + const graphQLError = new AbortError('GraphQL operation failed.', JSON.stringify(details, null, 2)) + graphQLError.details = details + throw graphQLError } throw error