diff --git a/.changeset/json-error-output.md b/.changeset/json-error-output.md new file mode 100644 index 00000000000..4b4d679f0ef --- /dev/null +++ b/.changeset/json-error-output.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': minor +--- + +Emit a parseable JSON error document on stdout instead of an error banner when `--json` is active diff --git a/docs/cli/error_handling.md b/docs/cli/error_handling.md index 6b80794027f..9006334987f 100644 --- a/docs/cli/error_handling.md +++ b/docs/cli/error_handling.md @@ -138,6 +138,83 @@ 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. +## Errors in JSON output + +When a command runs with JSON output active (`--json` or `-j`) and fails with a `FatalError`, the CLI writes a machine-readable error document instead of the human-readable banner. This is a public contract: scripts parse it, so treat changes to it as breaking. + +The shape of the document depends on `type`. Here is a typical `abort`, produced when the user can fix the problem themselves: + +```json +{ + "error": { + "type": "abort", + "message": "Couldn't find the app's configuration file", + "tryMessage": "Run shopify app config link to create one", + "nextSteps": ["Check that you're in the right directory"], + "customSections": [{"title": "Extensions", "body": [["name", "status"], ["my-ext", "failed"]]}] + } +} +``` + +A `bug` is the only type that carries `stack`, since it is the only type where users are asked to file a report: + +```json +{ + "error": { + "type": "bug", + "message": "Unexpected error while parsing the theme manifest", + "stack": "Error: ...\n at ..." + } +} +``` + +An `external` document additionally carries `command` and `args`, naming the subprocess that failed: + +```json +{ + "error": { + "type": "external", + "message": "npm install exited with a non-zero status", + "command": "npm", + "args": ["install"] + } +} +``` + +`tryMessage`, `nextSteps`, and `customSections` are optional on any of the three types; they are simply omitted above where a real document wouldn't include them. + +### The envelope + +Everything is nested under a single `error` key. That key is what lets a script tell failure from success without duck-typing: successful `--json` payloads are bare values (arrays, or objects shaped by the command), so an error object emitted at the top level would be ambiguous. This also matches oclif's and npm's own JSON error shape. + +### `type` + +A stable, machine-readable discriminator. It is one of: + +| `type` | Meaning | +| --- | --- | +| `abort` | An expected failure the user can act on (`AbortError`). | +| `bug` | An unexpected failure worth reporting to us (`BugError`, and anything unrecognised). | +| `external` | A subprocess the CLI invoked failed (`ExternalError`); `command` and `args` say which. | + +The values are string literals rather than class or enum names because the published bundle is minified with `minifyIdentifiers`, which rewrites class names. There is no `abortSilent` value: `AbortSilentError` exists to exit without printing anything, so it produces **no document at all** rather than a document describing itself. An unrecognised error is reported as `bug` rather than being mislabelled or emitted with an unknown `type`, so the list above is closed and safe to `switch` on exhaustively. + +### Field presence + +**Absent fields are omitted rather than emitted as `null`.** Use a presence check (`if ("nextSteps" in error)`), not a null comparison. Only `type` and `message` are guaranteed. + +`stack` appears **only when `type` is `bug`**, since that is the only type where users are asked to file a report. Do not rely on it for `abort` or `external`. + +### Streams and exit status + +The document goes to **stdout**; all human-readable output goes to **stderr**. Redirecting stdout to a file or a pipe therefore captures the document alone. + +**The process exit status is the status contract.** No `exitCode` field is emitted, deliberately: the error mapper constructs a fresh `AbortError` that does not carry oclif's exit code, so any code in the payload could disagree with the status the process actually exits with. Check the exit status, and use the document for the details. + +### One known approximation + +JSON mode has to be detected before oclif parses the arguments, so `sniffForJson` inspects `argv` directly. It errs towards enabling JSON: a `--json` that is really the *value* of a preceding value-taking flag (`shopify app info --path --json`) is treated as a request for JSON output. Distinguishing that case would mean knowing every flag's arity, which is oclif's parser rather than a sniff. + ## 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.test.ts b/packages/cli-kit/src/private/node/json-error.test.ts new file mode 100644 index 00000000000..3f0efd77d0d --- /dev/null +++ b/packages/cli-kit/src/private/node/json-error.test.ts @@ -0,0 +1,362 @@ +import {fatalErrorToJsonDocument, renderFatalErrorAsJson} from './json-error.js' +import {consoleLog, output} from './output.js' +import { + AbortError, + AbortSilentError, + BugError, + ExternalError, + FatalErrorType, + resolveJsonErrorType, +} from '../../public/node/error.js' +import {mockAndCaptureOutput} from '../../public/node/testing/output.js' +import {joinPath, moduleDirectory} from '../../public/node/path.js' +import {describe, expect, test, vi} from 'vitest' +import {transform} from 'esbuild' +import {readFile} from 'fs/promises' +import type {JsonErrorType} from '../../public/node/error.js' + +// Spies on the private `output` while keeping its real behaviour, which is what lets the +// stream-routing test below tell `outputResult` from `outputInfo`. The public `output.js` is +// deliberately left alone: it exports `collectedLogs` as a mutable binding, and spreading it +// into a mock would snapshot it, breaking `mockAndCaptureOutput` for every other test here. +vi.mock('./output.js', async (importOriginal) => { + const actual = await importOriginal() + return {...actual, output: vi.fn(actual.output)} +}) + +describe('the type discriminator', () => { + test('gives each error class a distinct, stable string', () => { + expect(fatalErrorToJsonDocument(new AbortError('boom'))?.error.type).toBe('abort') + expect(fatalErrorToJsonDocument(new BugError('boom'))?.error.type).toBe('bug') + expect(fatalErrorToJsonDocument(new ExternalError('boom', 'npm', ['install']))?.error.type).toBe('external') + }) + + test('distinguishes AbortError from ExternalError even though both carry FatalErrorType.Abort', () => { + // Given + const abort = new AbortError('boom') + const external = new ExternalError('boom', 'npm', ['install']) + + // Then + expect(abort.type).toBe(external.type) + expect(fatalErrorToJsonDocument(abort)?.error.type).not.toBe(fatalErrorToJsonDocument(external)?.error.type) + }) + + test('resolves every FatalErrorType member to its expected discriminator', () => { + // Given + // Spelled out rather than derived from `jsonErrorTypeForFatalErrorType`: deriving the + // expectation from the map under test would pass no matter what the map said. + const expectedForMember: Record = { + [FatalErrorType.Abort]: 'abort', + [FatalErrorType.AbortSilent]: 'abortSilent', + [FatalErrorType.Bug]: 'bug', + } + const numericMembers = Object.values(FatalErrorType).filter((member): member is FatalErrorType => { + return typeof member === 'number' + }) + + // Then + expect(numericMembers.length).toBeGreaterThan(0) + numericMembers.forEach((member) => { + // Fails loudly if a member is added to the enum without an expectation here, rather + // than silently checking nothing for it. + expect(expectedForMember[member]).toBeDefined() + expect(resolveJsonErrorType({type: member})).toBe(expectedForMember[member]) + }) + }) + + test('pins the enum values, which are a wire format shared across cli-kit copies', () => { + // `resolveJsonErrorType` reads this number off errors built by a possibly different copy + // of cli-kit, so reordering or renumbering a member silently changes what it means. + expect(FatalErrorType.Abort).toBe(0) + expect(FatalErrorType.AbortSilent).toBe(1) + expect(FatalErrorType.Bug).toBe(2) + }) + + test('falls back to the numeric type when jsonErrorType is missing, as for an error from another cli-kit copy', () => { + expect(resolveJsonErrorType({type: FatalErrorType.Abort})).toBe('abort') + expect(resolveJsonErrorType({type: FatalErrorType.Bug})).toBe('bug') + expect(resolveJsonErrorType({type: FatalErrorType.AbortSilent})).toBe('abortSilent') + }) + + test('reports an unrecognised numeric type as a bug rather than mislabelling it', () => { + // A member added by a newer cli-kit than the one resolving it. + expect(resolveJsonErrorType({type: 999 as FatalErrorType})).toBe('bug') + expect(resolveJsonErrorType({})).toBe('bug') + }) + + test('prefers an explicit jsonErrorType over the numeric type', () => { + expect(resolveJsonErrorType({type: FatalErrorType.Abort, jsonErrorType: 'external'})).toBe('external') + }) + + test('ignores an unknown explicit jsonErrorType from a newer cli-kit and falls back', () => { + // `jsonErrorType` is a writable public field and its type is erased at runtime, so a + // newer copy of cli-kit can set a discriminator this version does not advertise. Passing + // it through would put a value on the wire that consumers' exhaustive switches cannot + // handle, so it is ignored in favour of the numeric type. + const fromNewerCliKit = {jsonErrorType: 'new-type'} as unknown as {jsonErrorType: JsonErrorType} + + // Then + expect(resolveJsonErrorType({...fromNewerCliKit, type: FatalErrorType.Abort})).toBe('abort') + expect(resolveJsonErrorType({...fromNewerCliKit, type: FatalErrorType.Bug})).toBe('bug') + expect(resolveJsonErrorType(fromNewerCliKit)).toBe('bug') + }) + + test('never emits an unknown discriminator in a document', () => { + // Given + const error = {message: 'boom', type: FatalErrorType.Abort, jsonErrorType: 'new-type'} as unknown as Parameters< + typeof fatalErrorToJsonDocument + >[0] + + // When + const document = fatalErrorToJsonDocument(error) + + // Then + expect(JSON.stringify(document)).not.toContain('new-type') + expect(document?.error.type).toBe('abort') + }) +}) + +describe('flattening', () => { + test('flattens a TokenItem tryMessage to a plain string', () => { + // Given + const error = new AbortError('boom', ['Run', {command: 'shopify app dev'}, 'again']) + + // Then + expect(fatalErrorToJsonDocument(error)?.error.tryMessage).toBe('Run shopify app dev again') + }) + + test('flattens nextSteps to an array of strings', () => { + // Given + const error = new AbortError('boom', null, [ + ['Visit', {link: {label: 'the docs', url: 'https://shopify.dev'}}], + 'Try again', + ]) + + // Then + expect(fatalErrorToJsonDocument(error)?.error.nextSteps).toStrictEqual(['Visit the docs', 'Try again']) + }) + + test('flattens a token customSection body to a string and a tabularData body to a string matrix', () => { + // Given + const error = new AbortError('boom', null, undefined, [ + {title: 'Notes', body: ['See', {command: 'shopify help'}]}, + { + title: 'Extensions', + body: { + tabularData: [ + ['name', 'status'], + ['my-ext', {subdued: 'failed'}], + ], + }, + }, + ]) + + // When + const sections = fatalErrorToJsonDocument(error)?.error.customSections + + // Then + expect(sections).toStrictEqual([ + {title: 'Notes', body: 'See shopify help'}, + { + title: 'Extensions', + body: [ + ['name', 'status'], + ['my-ext', 'failed'], + ], + }, + ]) + }) + + test('omits the title of an untitled custom section', () => { + // Given + const error = new AbortError('boom', null, undefined, [{body: 'just a body'}]) + + // Then + expect(fatalErrorToJsonDocument(error)?.error.customSections).toStrictEqual([{body: 'just a body'}]) + }) + + test('strips ANSI escape codes from every string it emits', () => { + // Given + const red = (text: string) => `\u001b[31m${text}\u001b[39m` + const error = new AbortError( + red('boom'), + red('try this'), + [red('a step')], + [{title: 'Notes', body: red('a note')}, {body: {tabularData: [[red('cell')]]}}], + ) + + // When + const payload = fatalErrorToJsonDocument(error)?.error + + // Then + const serialized = JSON.stringify(payload) + expect(serialized).not.toContain('\u001b') + expect(payload?.message).toBe('boom') + expect(payload?.tryMessage).toBe('try this') + expect(payload?.nextSteps).toStrictEqual(['a step']) + expect(payload?.customSections).toStrictEqual([{title: 'Notes', body: 'a note'}, {body: [['cell']]}]) + }) +}) + +describe('field presence', () => { + test('omits absent optional fields rather than emitting nulls', () => { + // When + const payload = fatalErrorToJsonDocument(new AbortError('boom'))?.error + + // Then + expect(payload).toStrictEqual({type: 'abort', message: 'boom'}) + }) + + test('omits an empty nextSteps array rather than emitting []', () => { + // Given + const error = new AbortError('boom', null, []) + + // When + const payload = fatalErrorToJsonDocument(error)?.error + + // Then + expect(payload).not.toHaveProperty('nextSteps') + expect(payload).toStrictEqual({type: 'abort', message: 'boom'}) + }) + + test('omits an empty customSections array rather than emitting []', () => { + // Given + const error = new AbortError('boom', null, undefined, []) + + // When + const payload = fatalErrorToJsonDocument(error)?.error + + // Then + expect(payload).not.toHaveProperty('customSections') + expect(payload).toStrictEqual({type: 'abort', message: 'boom'}) + }) + + test('includes command and args for an ExternalError', () => { + // When + const payload = fatalErrorToJsonDocument(new ExternalError('boom', 'npm', ['install', '--save']))?.error + + // Then + expect(payload?.command).toBe('npm') + expect(payload?.args).toStrictEqual(['install', '--save']) + // `stack` is gated on the bug type, and `ExternalError` shares `FatalErrorType.Abort` + // with `AbortError`, so it must not leak a trace either. + expect(payload?.stack).toBeUndefined() + }) + + test('includes the stack for a bug, since that is the type users are asked to report', () => { + // When + const payload = fatalErrorToJsonDocument(new BugError('boom'))?.error + + // Then + expect(payload?.stack).toContain('boom') + }) + + test('omits the stack for an abort', () => { + // Given + const error = new AbortError('boom') + + // Then + expect(error.stack).toBeDefined() + expect(fatalErrorToJsonDocument(error)?.error.stack).toBeUndefined() + }) + + test('does not emit an exit code, because the mapped error does not carry oclif.exit', () => { + // When + const payload = fatalErrorToJsonDocument(new AbortError('boom'))?.error + + // Then + expect(payload).not.toHaveProperty('exitCode') + }) +}) + +describe('intentionally silent errors', () => { + test('emits no document for an AbortSilentError', () => { + expect(fatalErrorToJsonDocument(new AbortSilentError())).toBeUndefined() + }) + + test('emits no document for an AbortSilent type that arrived without a jsonErrorType', () => { + // An error built by a duplicate copy of cli-kit, where errorHandler's `instanceof` check + // does not hold and it reaches the serializer. + expect(fatalErrorToJsonDocument({type: FatalErrorType.AbortSilent, message: ''})).toBeUndefined() + }) + + test('writes nothing at all when the error is silent', () => { + // Given + const outputMock = mockAndCaptureOutput() + outputMock.clear() + + // When + renderFatalErrorAsJson(new AbortSilentError()) + + // Then + expect(outputMock.output()).toBe('') + }) +}) + +describe('renderFatalErrorAsJson', () => { + test('writes a single JSON.parse-able document', () => { + // Given + const outputMock = mockAndCaptureOutput() + outputMock.clear() + + // When + renderFatalErrorAsJson(new AbortError('boom', 'try this')) + + // Then + const written = outputMock.info() + expect(() => JSON.parse(written)).not.toThrow() + expect(JSON.parse(written)).toStrictEqual({error: {type: 'abort', message: 'boom', tryMessage: 'try this'}}) + }) + + test('nests the payload under a single error key so scripts can tell failure from success', () => { + // Given + const outputMock = mockAndCaptureOutput() + outputMock.clear() + + // When + renderFatalErrorAsJson(new AbortError('boom')) + + // Then + expect(Object.keys(JSON.parse(outputMock.info()))).toStrictEqual(['error']) + }) + + test('writes through the stdout logger rather than the stderr one', () => { + // The captured log cannot tell `outputResult` from `outputInfo`: both file their content + // under `info` via `collectLog`. What separates them is the logger they delegate to. + // `outputResult` calls the private `output()` with `consoleLog` (`process.stdout.write`), + // while `outputInfo` uses `consoleWarn` (`process.stderr.write`) and never calls + // `output()` at all, so asserting this call is what catches a switch between the two. + // Caveat kept deliberately: `shouldOutput()` is false under `isUnitTest()`, so nothing + // reaches a real stream here. This asserts the logger the write is handed to, which is + // the closest a unit test can get to observing stream identity. + // Given + const error = new AbortError('boom') + vi.mocked(output).mockClear() + + // When + renderFatalErrorAsJson(error) + + // Then + expect(output).toHaveBeenCalledWith(JSON.stringify(fatalErrorToJsonDocument(error)), 'info', consoleLog) + }) +}) + +describe('minification survival', () => { + // The published npm bundle is built with `minifyIdentifiers: true`, which is why the + // discriminator is a string literal rather than a class or enum name. vitest runs + // unminified TypeScript, so this is the only test that can catch a regression to a + // reflective name. A single-file transform keeps it fast and hermetic — no bundling. + test('keeps every discriminator string verbatim under minifyIdentifiers', async () => { + // Given + const errorSourcePath = joinPath(moduleDirectory(import.meta.url), '..', '..', 'public', 'node', 'error.ts') + const source = await readFile(errorSourcePath, 'utf8') + + // When + const {code} = await transform(source, {loader: 'ts', minifyIdentifiers: true}) + + // Then + ;['abort', 'abortSilent', 'bug', 'external'].forEach((discriminator) => { + expect(code).toContain(`"${discriminator}"`) + }) + }) +}) 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..16fe6761459 --- /dev/null +++ b/packages/cli-kit/src/private/node/json-error.ts @@ -0,0 +1,143 @@ +import {tokenItemToString, TokenItem} from './ui/components/TokenizedText.js' +import {outputResult, unstyled} from '../../public/node/output.js' +import {resolveJsonErrorType} from '../../public/node/error.js' +import type {TabularDataProps} from './ui/components/TabularData.js' +import type {EmittedJsonErrorType, JsonErrorType} from '../../public/node/error.js' +import type {AlertCustomSection} from '../../public/node/ui.js' + +/** + * A custom section flattened for JSON output. `body` is a plain string for token bodies and + * a row-major matrix of strings for tabular ones. + */ +interface JsonErrorCustomSection { + title?: string + body: string | string[][] +} + +/** + * The `error` payload of the JSON error document. + * + * Absent fields are omitted rather than emitted as `null`, so consumers can test with a + * simple presence check. `type` and `message` are always present. + */ +interface JsonErrorPayload { + type: EmittedJsonErrorType + message: string + tryMessage?: string + nextSteps?: string[] + customSections?: JsonErrorCustomSection[] + stack?: string + command?: string + args?: string[] +} + +/** + * The document written to stdout when a command fails with JSON output active. + * + * The single `error` key is what lets a script tell failure from success: successful + * payloads in this CLI are bare values (arrays, or objects shaped by the command), so a + * bare error object would force consumers to duck-type. This also matches oclif's and + * npm's own JSON error shape. + */ +export interface JsonErrorDocument { + error: JsonErrorPayload +} + +/** + * The subset of `FatalError` this module reads, with every field optional. + * + * Deliberately duck-typed rather than typed as `FatalError`: `isFatal` in `error.ts` also + * duck-types, so errors reaching us may come from a different copy or version of cli-kit + * and may not carry every field. + */ +interface FatalErrorLike { + message?: string + type?: number + jsonErrorType?: JsonErrorType + tryMessage?: TokenItem | null + nextSteps?: TokenItem[] + customSections?: AlertCustomSection[] + stack?: string + command?: string + args?: string[] +} + +function flattenTokenItem(token: TokenItem): string { + return unstyled(tokenItemToString(token)) +} + +function isTabularData(body: AlertCustomSection['body']): body is TabularDataProps { + return typeof body === 'object' && !Array.isArray(body) && 'tabularData' in body +} + +function customSectionToJson(section: AlertCustomSection): JsonErrorCustomSection { + const body = isTabularData(section.body) + ? section.body.tabularData.map((row) => row.map(flattenTokenItem)) + : flattenTokenItem(section.body) + + return { + ...(section.title === undefined ? {} : {title: section.title}), + body, + } +} + +/** + * Flattens a fatal error into the JSON document emitted when `--json` is active. + * + * Every nested `TokenItem` is reduced to a plain string with ANSI stripped; raw Ink/React + * props never reach the payload. No exit code is included: `errorMapper` builds a new + * `AbortError` that drops `oclif.exit`, so any code we read here could disagree with the + * status the process actually exits with. The process exit status is the contract. + * + * @param error - The fatal error to flatten. + * @returns The document to write, or `undefined` when the error is intentionally silent. + */ +export function fatalErrorToJsonDocument(error: FatalErrorLike): JsonErrorDocument | undefined { + const type = resolveJsonErrorType(error) + + // `AbortSilentError` exists to terminate the process without printing anything, usually + // after the user cancelled. Emitting a document for it would be a regression, so stay + // silent in JSON mode too. `errorHandler` normally returns before we get here; this is + // the fallback for an error built by a duplicate copy of cli-kit, where its `instanceof` + // check does not hold. + if (type === 'abortSilent') { + return undefined + } + + const tryMessage = error.tryMessage ?? undefined + const {nextSteps, customSections} = error + const hasNextSteps = nextSteps !== undefined && nextSteps.length > 0 + const hasCustomSections = customSections !== undefined && customSections.length > 0 + + return { + error: { + type, + message: unstyled(error.message ?? ''), + ...(tryMessage === undefined ? {} : {tryMessage: flattenTokenItem(tryMessage)}), + ...(hasNextSteps ? {nextSteps: nextSteps.map(flattenTokenItem)} : {}), + ...(hasCustomSections ? {customSections: customSections.map(customSectionToJson)} : {}), + // Only bugs ask the user to file a report, so they're the only type where a stack + // trace is worth the payload size. + ...(type === 'bug' && error.stack !== undefined ? {stack: unstyled(error.stack)} : {}), + ...(error.command === undefined ? {} : {command: error.command}), + ...(error.args === undefined ? {} : {args: error.args}), + }, + } +} + +/** + * Writes the JSON error document for a fatal error to stdout. + * + * Goes through `outputResult` rather than `process.stdout.write` for two reasons: it is the + * only stdout writer in the `output` family, so this matches how successful `--json` + * payloads are emitted; and it is the only route the `mockAndCaptureOutput` test harness + * can observe. + * + * @param error - The fatal error to write. + */ +export function renderFatalErrorAsJson(error: FatalErrorLike): void { + const document = fatalErrorToJsonDocument(error) + if (document !== undefined) { + outputResult(JSON.stringify(document)) + } +} diff --git a/packages/cli-kit/src/public/node/error-handler.test.ts b/packages/cli-kit/src/public/node/error-handler.test.ts index 6a6e5a1a14e..d10ff8ab6cc 100644 --- a/packages/cli-kit/src/public/node/error-handler.test.ts +++ b/packages/cli-kit/src/public/node/error-handler.test.ts @@ -7,6 +7,7 @@ import {hashString} from './crypto.js' import {isLocalEnvironment} from '../../private/node/context/service.js' import {getLastSeenUserIdAfterAuth} from '../../private/node/session.js' import {GraphQLClientError} from '../../private/node/api/headers.js' +import {environmentVariables} from '../../private/node/constants.js' import {settings} from '@oclif/core' import {beforeEach, describe, expect, test, vi} from 'vitest' @@ -48,6 +49,12 @@ vi.mock('@oclif/core', () => ({ debug: false, }, Interfaces: {}, + // `errorMapper` checks `error instanceof Errors.CLIError`. Tests whose error reaches it + // (rather than being short-circuited by the CancelExecution/AbortSilentError early returns) + // need this to be a real constructor. + Errors: { + CLIError: class CLIError extends Error {}, + }, })) beforeEach(() => { @@ -104,6 +111,88 @@ describe('errorHandler', async () => { }) }) +describe('errorHandler with JSON output active', () => { + /** + * Enables JSON output through the real detection path — `SHOPIFY_FLAG_JSON` rather than a + * mock — so these tests also cover the env-var half of `jsonOutputEnabled`. The variable is + * always removed, even if an assertion fails, to keep tests independent. + */ + async function withJsonOutputEnabled(runTest: () => Promise): Promise { + process.env[environmentVariables.json] = '1' + try { + await runTest() + } finally { + delete process.env[environmentVariables.json] + } + } + + test('still reports the error to Bugsnag after writing the JSON document', async () => { + await withJsonOutputEnabled(async () => { + // Given + vi.spyOn(process, 'exit').mockResolvedValue(null as never) + const outputMock = mockAndCaptureOutput() + outputMock.clear() + + // When + await errorHandler(new error.BugError('boom')) + + // Then + expect(JSON.parse(outputMock.info()).error.type).toBe('bug') + expect(onNotify).toHaveBeenCalledOnce() + }) + }) + + test('still reports the error to Bugsnag when serializing the JSON document throws', async () => { + await withJsonOutputEnabled(async () => { + // Given + vi.spyOn(process, 'exit').mockResolvedValue(null as never) + const outputMock = mockAndCaptureOutput() + outputMock.clear() + // A malformed token makes `tokenItemToString` throw, which is the realistic way + // serialization can fail. + const bugError = new error.BugError('boom', {unrecognisedToken: true} as unknown as string) + + // When + await errorHandler(bugError) + + // Then + expect(outputMock.info()).toBe('') + expect(onNotify).toHaveBeenCalledOnce() + }) + }) + + test('keeps CancelExecution on its existing human-readable path', async () => { + await withJsonOutputEnabled(async () => { + // Given + vi.spyOn(process, 'exit').mockResolvedValue(null as never) + const outputMock = mockAndCaptureOutput() + outputMock.clear() + + // When + await errorHandler(new error.CancelExecution('Custom message')) + + // Then + expect(outputMock.info()).toMatch('✨ Custom message') + expect(() => JSON.parse(outputMock.info())).toThrow() + }) + }) + + test('keeps AbortSilentError silent', async () => { + await withJsonOutputEnabled(async () => { + // Given + vi.spyOn(process, 'exit').mockResolvedValue(null as never) + const outputMock = mockAndCaptureOutput() + outputMock.clear() + + // When + await errorHandler(new error.AbortSilentError()) + + // Then + expect(outputMock.output()).toBe('') + }) + }) +}) + describe('bugsnag stack cleaning', () => { test.each([ ['dependency in relative path', 'cool-project/node_modules/deppy/foo/bar.ts', 'deppy/foo/bar.ts'], diff --git a/packages/cli-kit/src/public/node/error.test.ts b/packages/cli-kit/src/public/node/error.test.ts index b80d0054526..a8fb6c3af8b 100644 --- a/packages/cli-kit/src/public/node/error.test.ts +++ b/packages/cli-kit/src/public/node/error.test.ts @@ -1,5 +1,15 @@ -import {AbortError, BugError, handler, cleanSingleStackTracePath, shouldReportErrorAsUnexpected} from './error.js' +import { + AbortError, + AbortSilentError, + BugError, + ExternalError, + handler, + cleanSingleStackTracePath, + shouldReportErrorAsUnexpected, +} from './error.js' import {renderFatalError} from './ui.js' +import {jsonOutputEnabled} from './environment.js' +import {mockAndCaptureOutput} from './testing/output.js' import {ClientError} from 'graphql-request' import {describe, expect, test, vi} from 'vitest' @@ -9,6 +19,19 @@ function clientError(status: number, code?: string): ClientError { } vi.mock('./ui.js') +vi.mock('./environment.js') + +/** + * `jsonOutputEnabled` reads `process.argv` and the environment, both of which are global. + * Mocking it keeps these tests independent of how vitest was invoked; the detection logic + * itself is covered by the `sniffForJson` tests in `path.test.ts`. + */ +function givenJsonOutputIs(enabled: boolean): ReturnType { + vi.mocked(jsonOutputEnabled).mockReturnValue(enabled) + const outputMock = mockAndCaptureOutput() + outputMock.clear() + return outputMock +} describe('handler', () => { test('error output uses same input error instance when the error type is abort', async () => { @@ -49,6 +72,126 @@ describe('handler', () => { }) }) +describe('handler with JSON output active', () => { + test('writes a JSON error document and renders no banner', async () => { + // Given + const outputMock = givenJsonOutputIs(true) + + // When + await handler(new AbortError('boom', 'try this')) + + // Then + expect(JSON.parse(outputMock.info())).toStrictEqual({ + error: {type: 'abort', message: 'boom', tryMessage: 'try this'}, + }) + expect(renderFatalError).not.toHaveBeenCalled() + expect(outputMock.error()).toBe('') + }) + + test('renders the banner and no JSON when JSON output is inactive', async () => { + // Given + const outputMock = givenJsonOutputIs(false) + const error = new AbortError('boom') + + // When + await handler(error) + + // Then + expect(renderFatalError).toHaveBeenCalledWith(error) + expect(outputMock.info()).toBe('') + }) + + test.each([ + ['a string', 'a plain string failure', 'a plain string failure'], + ['an Error', new Error('a real error'), 'a real error'], + ['a non-Error object', {message: 'a duck-typed failure'}, 'a duck-typed failure'], + ['an object with no message at all', {}, 'Unknown error'], + ])('reports %s thrown by a command as a bug', async (_label, thrown, expectedMessage) => { + // Given + const outputMock = givenJsonOutputIs(true) + + // When + await handler(thrown) + + // Then + const {error} = JSON.parse(outputMock.info()) + expect(error.type).toBe('bug') + expect(error.message).toBe(expectedMessage) + }) + + test('distinguishes an ExternalError from an AbortError', async () => { + // Given + const outputMock = givenJsonOutputIs(true) + + // When + await handler(new ExternalError('boom', 'npm', ['install'])) + + // Then + expect(JSON.parse(outputMock.info()).error).toStrictEqual({ + type: 'external', + message: 'boom', + command: 'npm', + args: ['install'], + }) + }) + + test('stays silent for an AbortSilentError, which exists to print nothing', async () => { + // Given + const outputMock = givenJsonOutputIs(true) + + // When + await handler(new AbortSilentError()) + + // Then + expect(outputMock.output()).toBe('') + expect(renderFatalError).not.toHaveBeenCalled() + }) + + test('writes the document before resolving, so it lands before oclif calls process.exit', async () => { + // `BaseCommand.catch` awaits `errorHandler` and only then calls `Errors.handle`, which + // exits the process. The document being present the moment `handler` resolves is + // therefore what guarantees it is never lost to the exit. + // Given + const outputMock = givenJsonOutputIs(true) + + // When + await handler(new AbortError('boom')) + + // Then + expect(outputMock.info()).not.toBe('') + }) + + test('falls back to the banner and still resolves when serialization throws', async () => { + // A throw here must not propagate: `errorHandler` reports the error to analytics only + // after `handler` resolves, so rethrowing would silently kill crash reporting. + // A malformed token is the realistic trigger: `tokenItemToString` falls through to its + // array branch for an unrecognised shape and throws on `.map`. + // Given + const outputMock = givenJsonOutputIs(true) + const error = new AbortError('boom', {unrecognisedToken: true} as unknown as string) + + // When + await expect(handler(error)).resolves.toBe(error) + + // Then + expect(renderFatalError).toHaveBeenCalledWith(error) + expect(outputMock.info()).toBe('') + }) + + test('leaves the exit code oclif will use untouched', async () => { + // Given + givenJsonOutputIs(true) + const error = new AbortError('boom') as AbortError & {oclif: {exit: number}} + error.oclif = {exit: 2} + + // When + await handler(error) + + // Then + expect(error.oclif.exit).toBe(2) + }) +}) + describe('stack file path helpers', () => { test.each([ ['simple file:///', 'file:///something/there.js'], diff --git a/packages/cli-kit/src/public/node/error.ts b/packages/cli-kit/src/public/node/error.ts index 9f84774cef9..b2ec21055ec 100644 --- a/packages/cli-kit/src/public/node/error.ts +++ b/packages/cli-kit/src/public/node/error.ts @@ -1,5 +1,6 @@ import {normalizePath} from './path.js' -import {OutputMessage, stringifyMessage, TokenizedString} from './output.js' +import {jsonOutputEnabled} from './environment.js' +import {outputDebug, OutputMessage, stringifyMessage, TokenizedString} from './output.js' import {InlineToken, TokenItem, tokenItemToString} from '../../private/node/ui/components/TokenizedText.js' import {hasRateLimitCode} from '../../private/node/analytics/graphql-error-codes.js' @@ -10,10 +11,112 @@ import type {AlertCustomSection} from './ui.js' export {ExtendableError} from 'ts-error' +/** + * How the program should behave when a `FatalError` reaches the top-level handler. + * + * The values are written out explicitly because they are effectively a cross-version wire + * format: `resolveJsonErrorType` reads this number off errors that may have been built by a + * different copy of cli-kit (see `bin/bundling/esbuild-plugin-dedup-cli-kit.js`), so a given + * number has to keep meaning the same thing across versions. This list is append-only: add + * new members with new values, and never reorder or renumber the existing ones. + */ export enum FatalErrorType { - Abort, - AbortSilent, - Bug, + Abort = 0, + AbortSilent = 1, + Bug = 2, +} + +/** + * Every `JsonErrorType`, as a runtime value. + * + * `JsonErrorType` is derived from this array rather than declared separately so that the + * compile-time union and the runtime allow-list in `isJsonErrorType` cannot drift apart. + */ +const jsonErrorTypes = ['abort', 'abortSilent', 'bug', 'external'] as const + +/** + * Stable, machine-readable classification of a fatal error, used to derive the `error.type` + * field of the JSON error document produced when `--json` is active. + * + * Not every member reaches the wire: see `EmittedJsonErrorType` for the subset that can + * actually appear in a document. + * + * These are string literals rather than class or enum names on purpose: the published npm + * bundle is built with `minifyIdentifiers: true` (see `packages/cli/bin/bundle.js`), which + * rewrites `constructor.name` to a single letter that changes between builds. String + * literals survive minification. + * + * `FatalErrorType` on its own is too coarse to use here, because `AbortError` and + * `ExternalError` both carry `FatalErrorType.Abort`. + */ +export type JsonErrorType = (typeof jsonErrorTypes)[number] + +/** + * The `JsonErrorType` values that can appear as `error.type` in an emitted document. + * + * `abortSilent` is an internal classification only: `AbortSilentError` exists to terminate + * the process without printing anything, so no document is emitted for it at all. Excluding + * it here keeps the emitted discriminator a closed union that consumers can switch on + * exhaustively without handling a value they can never receive. + */ +export type EmittedJsonErrorType = Exclude + +/** + * Whether a value is one of the discriminators this version of cli-kit knows about. + * + * @param value - The value to check. + * @returns Whether the value is a known `JsonErrorType`. + */ +function isJsonErrorType(value: unknown): value is JsonErrorType { + return typeof value === 'string' && (jsonErrorTypes as ReadonlyArray).includes(value) +} + +/** + * The `JsonErrorType` each `FatalErrorType` maps to by default. Subclasses needing a + * finer-grained discriminator than the enum can express override `jsonErrorType` in their + * own constructor. + * + * The `satisfies` clause is the exhaustiveness guard: adding a member to `FatalErrorType` + * without giving it a discriminator here is a compile error. + */ +const jsonErrorTypeForFatalErrorType = { + [FatalErrorType.Abort]: 'abort', + [FatalErrorType.AbortSilent]: 'abortSilent', + [FatalErrorType.Bug]: 'bug', +} as const satisfies Record + +/** + * The fields `resolveJsonErrorType` needs, both optional. + * + * `isFatal` duck-types on the presence of `type` rather than using `instanceof`, so an error + * can reach us having been built by a different copy of cli-kit (see + * `bin/bundling/esbuild-plugin-dedup-cli-kit.js`) that predates `jsonErrorType`, or carrying + * an enum member this version doesn't know about. + */ +interface JsonErrorTypeSource { + type?: FatalErrorType + jsonErrorType?: JsonErrorType +} + +/** + * Resolves the JSON classification for a fatal error. + * + * Anything unrecognised is reported as a bug rather than silently mislabelled. + * + * @param error - The error to resolve a classification for. + * @returns The classification for the error. + */ +export function resolveJsonErrorType(error: JsonErrorTypeSource): JsonErrorType { + // `jsonErrorType` is validated rather than trusted: the type is erased at compile time and + // the field is publicly writable, so an error built by a newer copy of cli-kit can carry a + // discriminator this version has never heard of. Passing it through would put an + // unadvertised value on the wire and break the closed union consumers switch on, so an + // unknown value is ignored in favour of the numeric fallback. + if (isJsonErrorType(error.jsonErrorType)) { + return error.jsonErrorType + } + const knownTypes: Record = jsonErrorTypeForFatalErrorType + return (error.type === undefined ? undefined : knownTypes[error.type]) ?? 'bug' } export class CancelExecution extends Error {} @@ -25,6 +128,7 @@ export class CancelExecution extends Error {} export abstract class FatalError extends Error { tryMessage: TokenItem | null type: FatalErrorType + jsonErrorType: JsonErrorType nextSteps?: TokenItem[] formattedMessage?: TokenItem customSections?: AlertCustomSection[] @@ -61,6 +165,7 @@ export abstract class FatalError extends Error { } this.type = type + this.jsonErrorType = jsonErrorTypeForFatalErrorType[type] this.nextSteps = nextSteps this.customSections = customSections this.skipOclifErrorHandling = true @@ -104,6 +209,9 @@ export class ExternalError extends FatalError { tryMessage: TokenItem | OutputMessage | null = null, ) { super(message, FatalErrorType.Abort, tryMessage) + // `FatalErrorType.Abort` is shared with `AbortError`, so override the discriminator to + // keep the two distinguishable in JSON output. + this.jsonErrorType = 'external' this.command = command this.args = args } @@ -149,8 +257,29 @@ export async function handler(error: unknown): Promise { } } - const {renderFatalError} = await import('./ui.js') - renderFatalError(fatal) + // This is the single choke point for fatal error rendering, so it's the only place that + // can guarantee exactly one JSON document per invocation. The other `renderFatalError` + // call sites are either outside the command lifecycle (the `uncaughtException` handlers) + // or render and carry on inside long-running dev servers, where emitting a document each + // time would produce unparseable output. + let renderedAsJson = false + if (jsonOutputEnabled()) { + try { + const {renderFatalErrorAsJson} = await import('../../private/node/json-error.js') + renderFatalErrorAsJson(fatal) + renderedAsJson = true + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (serializationError) { + // A failure to serialize must not hide the error or stop the caller from reporting it + // to analytics, so fall through to the human-readable banner instead of rethrowing. + outputDebug(`Failed to render the error as JSON: ${serializationError}`) + } + } + + if (!renderedAsJson) { + const {renderFatalError} = await import('./ui.js') + renderFatalError(fatal) + } return Promise.resolve(error) } diff --git a/packages/cli-kit/src/public/node/path.test.ts b/packages/cli-kit/src/public/node/path.test.ts index 4972b3ac276..c5594e88b75 100644 --- a/packages/cli-kit/src/public/node/path.test.ts +++ b/packages/cli-kit/src/public/node/path.test.ts @@ -1,4 +1,4 @@ -import {relativizePath, normalizePath, cwd, sniffForPath, commonParentDirectory} from './path.js' +import {relativizePath, normalizePath, cwd, sniffForPath, sniffForJson, commonParentDirectory} from './path.js' import {describe, test, expect} from 'vitest' describe('relativize', () => { @@ -93,3 +93,36 @@ describe('sniffForPath', () => { expect(path).toStrictEqual('/path/to/project') }) }) + +describe('sniffForJson', () => { + test.each([ + ['the --json flag', ['node', 'shopify', 'app', 'info', '--json']], + ['the -j short flag', ['node', 'shopify', 'app', 'info', '-j']], + ['a cluster ending in j, which oclif parses as -v -j', ['node', 'shopify', 'app', 'info', '-vj']], + ['a cluster starting with j', ['node', 'shopify', 'app', 'info', '-jv']], + ['--json before a passthrough separator', ['node', 'shopify', 'app', 'info', '--json', '--', 'extra']], + ])('returns true for %s', (_label, argv) => { + expect(sniffForJson(argv)).toBe(true) + }) + + test.each([ + ['no JSON flag at all', ['node', 'shopify', 'app', 'info']], + ['a cluster of short flags without j', ['node', 'shopify', 'app', 'info', '-vf']], + ['a bare dash, which is a positional argument rather than a flag', ['node', 'shopify', 'app', 'info', '-']], + ['--json after the passthrough separator', ['node', 'shopify', 'app', 'function', 'run', '--', '--json']], + ['-j after the passthrough separator', ['node', 'shopify', 'app', 'function', 'run', '--', '-j']], + ['-vj after the passthrough separator', ['node', 'shopify', 'app', 'function', 'run', '--', '-vj']], + ['a value that merely contains json', ['node', 'shopify', 'app', 'info', '--path', 'my-json-app']], + ])('returns false for %s', (_label, argv) => { + expect(sniffForJson(argv)).toBe(false) + }) + + test('reports JSON output as enabled when --json is really the value of a preceding flag', () => { + // A documented false positive rather than a bug: telling this apart from a genuine + // `--json` would mean knowing that `--path` takes a value, and every other flag's arity + // with it, which is oclif's parser rather than a sniff. Erring towards JSON keeps machine + // output parseable; the cost is a JSON error document for a command that never asked for + // one, which a human reading the terminal sees rather than a script. + expect(sniffForJson(['node', 'shopify', 'app', 'info', '--path', '--json'])).toBe(true) + }) +}) diff --git a/packages/cli-kit/src/public/node/path.ts b/packages/cli-kit/src/public/node/path.ts index f3721780110..f899aa9d9c6 100644 --- a/packages/cli-kit/src/public/node/path.ts +++ b/packages/cli-kit/src/public/node/path.ts @@ -199,11 +199,24 @@ export function sniffForPath(argv = process.argv): string | undefined { /** * Returns whether the `--json` or `-j` flags are present in the arguments. * + * This is a deliberate approximation of oclif's own `jsonEnabled()`, which can't be called + * here because we run before parsing. It errs towards reporting JSON output as enabled: a + * `--json` consumed as the *value* of a preceding value-taking flag (`--path --json`) is + * counted as a request for JSON, because recognising it as a value would mean knowing every + * flag's arity, which is oclif's parser rather than a sniff. + * * @param argv - The arguments to search for the `--json` and `-j` flags. * @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') + // Everything after the `--` separator is a passthrough argument rather than a flag, so + // `shopify app function run -- --json` is not a request for JSON output. + const passthroughIndex = argv.indexOf('--') + const flags = passthroughIndex === -1 ? argv : argv.slice(0, passthroughIndex) + // `-j` is an alias for `--json`, and oclif accepts clustered short flags, so `-vj` enables + // JSON output just as much as `-j` does. The character class excludes a bare `-` and the + // long form, which is why `--json` is still matched explicitly. + return flags.some((token) => token === '--json' || (/^-[a-zA-Z]+$/.test(token) && token.includes('j'))) } /**