fix(cmdutils): write a JSON error object when --output=json

ANBUCHELVAN GANESAN CSErequested to merge
gitlab-community/gitlab-org/cli:anbu-8138-json-error-output into main

Description

Opened as a Draft on purpose: !2801 (closed) was moved back to draft pending consensus in #8138, and that consensus has not been recorded yet. This MR implements @timofurrer's proposal from #8138 (comment 3054017698) so there is something concrete to react to, and it flags the one question that is still open below.

The bug

--output=json (-F json) only affects the success path. When a command fails, stdout gets nothing at all and the failure is reported solely through Fang's styled error box on stderr — in both TTY and non-TTY runs. A caller that pipes glab into a JSON parser sees an empty stream.

$ glab variable get DUMMY -F json          # before
STDOUT: (empty)
STDERR:
          
   ERROR  
          
  404 Not Found.

What this changes

Following the proposal's first iteration:

  1. The human-readable error still goes to stderr in every output mode. Unchanged — this is what clig.dev asks for and what kubectl / gh / docker / aws all do.
  2. A machine-readable object now goes to stdout when JSON was requested.
$ glab variable get DUMMY -F json          # after
STDOUT:
{"error":{"message":"404 Not Found"}}
STDERR: (the same error box as before)
$ echo $?
1

How it works

Output format is per-command state (each of the 55 cmdutils.EnableJSONOutput call sites binds its own *string), and Fang hands the global error handler only the error — never the *cobra.Command — so the handler had no way to know what the user asked for.

  • IOStreams gains an unexported outputFormat plus SetOutputFormat / IsJSONOutput, mirroring how the equally cross-cutting --jq filter already lives on IOStreams.
  • cmdutils.EnableJSONOutput wraps the enum flag value so cobra records the format on IOStreams while parsing flags. That is deliberately earlier than a PreRunE hook: it means the format is known even when the command fails during argument validation, and it cannot be clobbered by a command that sets its own PreRunE later. The wrapper still satisfies cmdutils.AllowedValuer, so the MCP tool schema keeps publishing the enum (covered by a test).
  • cmdutils.GitLabErrorHandler becomes cmdutils.NewGitLabErrorHandler(streams), closing over the shared IOStreams from cmd/glab/main.go.
  • The object itself is written by a new IOStreams.PrintJSONError helper (issue proposal item 3, "maybe with a helper wrapped in iostreams"), so every command that uses EnableJSONOutput gets the behaviour without a per-command change.

SilentError, the interrupted-command message, exit codes, and all text-mode output are unchanged.

Known scope boundary: SilentError still yields an empty stdout under -F json — the same symptom this MR otherwise closes. I checked its one use alongside EnableJSONOutput, internal/commands/ci/status/status.go:294 (returned from inside the --wait/--live polling loop when the pipeline ends up failed) — that command's own --output flag description already says "JSON output is not compatible with --live, --wait, or --compact flags," so this isn't a reachable combination today. I didn't special-case SilentError in the handler itself; flagging this in case there's another caller I didn't find, or if that's judged worth handling generically anyway.

The open question, and why I picked this branch

Item 2 of the proposal offered two options, and #8138 never settled on one:

  • (a) a well-defined machine-readable error object on stdout — what this MR implements.
  • (b) a zero-value object matching the success shape ([], null, {}) — @phikai's preference, "with the benefit to not break downstream parsing".

I implemented (a) because (b) cannot be centralised. The zero value depends on each command's success shape — [] for securefile list, an object for variable get — which is exactly the knowledge the global error handler does not have. Doing (b) would mean touching all 55 commands to declare a zero value, and would still leave glab variable get MISSING -F json printing {}, which a caller cannot distinguish from a real empty result. Happy to switch if you'd rather have (b) — it is a bigger, more mechanical change and I'd want the decision recorded on the issue first.

On the shape: the message is nested under error rather than flat so that (i) it cannot be mistaken for the success payload written to the same stream, and (ii) a status / machine-readable reason can be added later without changing the type of an existing field. I did not attempt an RFC 9457 mapping — type/instance have no honest CLI meaning, and a partial mapping seemed worse than a small explicit shape. Deferring to you on this.

--jq is deliberately not applied to the error object: the expression targets the command's success output, so running it here would replace the failure the user needs to see with a filter error.

Related Issues

Relates to #8138

I used Relates to rather than Resolves on purpose — this is the first iteration of a multi-part proposal, so I don't think it should auto-close the issue. Also related: !2801 (closed) (the earlier attempt, closed).

How has this been tested?

Automated — all new, no existing tests changed or removed:

  • internal/cmdutils/output_format_test.go (new, 8 tests) — the format is recorded for --output json / -F text, is not recorded when the flag is absent or invalid, is recorded even when argument validation fails, and the flag still implements AllowedValuer for the MCP schema.
  • internal/cmdutils/errors_test.go (new, 8 tests) — text mode leaves stdout empty; JSON mode writes the object to stdout and the human-readable error to stderr; SilentError stays silent in both modes; context.Canceled maps to the interrupted message in both modes; --jq is ignored for errors. Plus a table-driven end-to-end test that drives a real cobra.Command wired through EnableJSONOutput and feeds the resulting error to the handler the way Fang does.
  • internal/iostreams/print_json_test.go (3 added) — PrintJSONError envelope shape, --jq bypass, and the nil-JQ path.

Manual, with the real binary against a local HTTP server returning 404, run both inside a pty and piped:

invocation stdout stderr exit
variable get DUMMY -F json {"error":{"message":"404 Not Found"}} error box 1
variable get DUMMY (empty) error box 1
variable get DUMMY -F text (empty) error box 1
variable get -F json (missing arg) {"error":{"message":"accepts between 1 and 1 arg(s), received 0"}} error box 1
variable get OKVAR -F json (success) the variable object, as before (empty) 0
variable get OKVAR -F json --jq .value hello, as before (empty) 0
variable get DUMMY -F json --jq .value {"error":{"message":"404 Not Found"}} error box 1
variable get DUMMY -F bogus (empty) must be one of [text json] 1

Repository-wide checks, since EnableJSONOutput has 55 call sites:

  • go build ./... — clean.
  • go vet ./... — clean.
  • gofmt -l . — empty.
  • golangci-lint run ./... (v2.12.2, per .tool-versions) — 0 issues.
  • go test ./... — all 285 packages pass, no pre-existing failures. Run with make test's environment (VISUAL/EDITOR/PAGER/GITLAB_TOKEN cleared, CI_PROJECT_PATH set).
  • go run ./cmd/gen-docs/docs.go — no diff; this MR changes no command help text.

Not verified locally: lefthook is not installed here, so the full pre-push chain (markdownlint, vale, lychee) did not run — no Markdown or docs files are touched by this MR, so I expect those to be no-ops. The commit-msg hook was validated by running npx commitlint --config .commitlintrc.js against the commit message directly.

Screenshots (if appropriate):

n/a — the before/after is the table above.

Edited by ANBUCHELVAN GANESAN CSE

Merge request reports