Emit a JSON error document when --json is active - #8182
Draft
amcaplan wants to merge 2 commits into
Draft
Conversation
Failing commands rendered only a human error banner, which goes to
stderr, so `shopify <command> --json | jq` received nothing parseable on
stdout. Route fatal errors through a JSON serializer when JSON output is
active, emitting a single `{"error": {...}}` document on stdout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
The example showed `stack` plus `command`/`args` on an `abort` document, but `stack` is emitted only for `bug` and `command`/`args` come from `ExternalError`. Split it into one example per type so no example shows a combination a real document cannot produce. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationspackages/cli-kit/dist/private/node/json-error.d.tsimport { TokenItem } from './ui/components/TokenizedText.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[];
}
/**
* 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 declare function fatalErrorToJsonDocument(error: FatalErrorLike): JsonErrorDocument | undefined;
/**
* 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 declare function renderFatalErrorAsJson(error: FatalErrorLike): void;
export {};
Existing type declarationspackages/cli-kit/dist/public/node/error.d.ts@@ -2,11 +2,73 @@ import { OutputMessage } from './output.js';
import { InlineToken, TokenItem } from '../../private/node/ui/components/TokenizedText.js';
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 declare enum FatalErrorType {
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.
+ */
+declare const jsonErrorTypes: readonly ["abort", "abortSilent", "bug", "external"];
+/**
+ * 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<JsonErrorType, 'abortSilent'>;
+/**
+ * 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 declare function resolveJsonErrorType(error: JsonErrorTypeSource): JsonErrorType;
export declare class CancelExecution extends Error {
}
/**
@@ -16,6 +78,7 @@ export declare class CancelExecution extends Error {
export declare abstract class FatalError extends Error {
tryMessage: TokenItem | null;
type: FatalErrorType;
+ jsonErrorType: JsonErrorType;
nextSteps?: TokenItem<InlineToken>[];
formattedMessage?: TokenItem;
customSections?: AlertCustomSection[];
packages/cli-kit/dist/public/node/path.d.ts@@ -124,6 +124,12 @@ export declare function sniffForPath(argv?: string[]): 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.
*/
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
--jsonwas never considered by the global error renderer. Because the human error banner is written to stderr, a failingshopify <command> --json | jqpreviously received nothing on stdout — not malformed output, but no output at all. Scripts had no parseable failure signal.This routes fatal errors through a JSON serializer when JSON output is active, emitting exactly one document on stdout.
WHAT is this pull request doing?
Shape
{"error":{"type":"abort","message":"Couldn't find an app","tryMessage":"Run shopify app init","nextSteps":["Visit the docs"],"customSections":[{"title":"Extensions","body":[["name","status"],["my-ext","failed"]]}]}}{"error": ...}envelope. Successful--jsonpayloads in this CLI are bare values —theme listandapp versions listemit arrays,theme pushandstore listemit command-shaped objects. A top-levelerrorkey is therefore unambiguous, and no existing success payload uses one. It also matches oclif's and npm's convention.typeis a closed set:abort,bug,external. Machine-stable by design (see below).null— consumers use presence checks, consistent with existingJSON.stringify/conditional-spread practice in the repo.stackonly appears forbug, the one type that asks the user to file a report.exitCodefield.errorMapperconstructs a freshAbortErrorthat dropsoclif.exit, so any code in the payload could contradict the real status. The process exit status is the contract.TokenItemis flattened to a plain string with ANSI stripped; no Ink/React props reach the payload.Why the discriminator is a string literal
Not
constructor.nameand noterror.name.packages/cli/bin/bundle.jsbuilds the published npm bundle withminifyIdentifiers: trueand nokeepNames, soclass AbortErrorbecomese=class extends s{}— any reflective name is garbage in production. (error.nameis worse: no error class inerror.tsassigns it, and ES subclasses don't inherit it, so it is"Error"for every type.) Each class carries an explicit literal, backed by asatisfies Record<FatalErrorType, JsonErrorType>map so adding aFatalErrorTypemember fails the build. A test transformserror.tswithminifyIdentifiers: trueand asserts every literal survives.Known approximations
sniffForJsoninspectsprocess.argvbefore oclif parses, because the error path must work even when parsing itself failed. It respects the--passthrough boundary and handles clustered short flags. It cannot model per-flag arity, so--path --json(where--jsonis consumed as another flag's value) is a false positive. Fixing that means reimplementing oclif's parser in the error path; not worth the fragility.theme preview -jis a permanent false positive — that command declares-jfor an unrelated flag. Blast radius is limited to printing a JSON error instead of a banner on a failingtheme preview.renderConcurrentwrites Ink frames to stdout. No command pairs it with--jsontoday (app build/app deploydeclare no json flag); onlytheme-command.ts's multi-environment runner remains, where output is already interleaved across environments. Gating the progress renderers on JSON mode is a sensible hardening follow-up, not a fix this change needs.renderTasksalready defaults to stderr.Review provenance
Three independent reviews on three different models audited this before submission (control flow, public contract, test integrity). They produced one blocker and several fixes, all addressed: the explicit discriminator is now runtime-validated so an unadvertised value from a foreign cli-kit copy cannot reach the wire;
FatalErrorType's numeric values are pinned as append-only since they act as a cross-copy wire format;abortSilentcan no longer appear in an emitted document's type; and two false-green tests were rewritten to be load-bearing and mutation-verified.Follow-ups (deliberately out of scope)
uncaughtExceptionhandlers (bootstrap.ts,index.ts) still render human output.plugin-did-you-meanare ungated.renderConcurrent) on JSON mode.launchCLIdouble-prints errors (missingskipOclifErrorHandling).error-grouping.tskeys Bugsnag'sgroupingHashonconstructor.name, which minification mangles.theme previewdeclaresjsonwithoutchar: 'j', inconsistent with the global flag.How to test your changes?
Testing note — an honest limitation
Stream identity is not directly assertable in this harness:
shouldOutput()returnsfalseunderisUnitTest(), so nothing reaches a real stream, andcollectLogkeys captured output by log level rather than by stream — sooutputResult(stdout) andoutputInfo(stderr) both land underinfoand the captured log cannot tell them apart.What the suite asserts instead is the logger reference:
outputResultdelegates to the privateoutput()withconsoleLog(process.stdout.write), whileoutputInfousesconsoleWarnand never callsoutput()at all. Asserting that call is what catches a switch between the two. That is the closest a unit test here can get to observing stream identity; verifying real file-descriptor separation needs an integration-level test.This distinction is not academic. The original version of that test asserted only that captured output was non-empty — and mutating
outputResulttooutputInfoleft 27 of 28 tests in the file green. Only the rewritten test catches it.Testing
cli-kitsuite: 146 files, 1735 passed | 1 skipped, exit 0--skip-nx-cache, since a cached pass would prove nothing)pnpm eslint srcclean, 0 errors 0 warningsChecklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add