--json output is not safe for jq: errors and warnings go to stdout

[blsmth]

Found while auditing #166 for further fallout from the --json flag added in #137. The shorthand collision is fixed (#166) and config list -j is restored (#170), but the --json output contract itself is not safe for jq yet.

All line numbers are against main at the time of writing.

1. Errors go to stdout, not stderr — corrupts every --json stream on failure

cmd/root.go:144

func printError(text string) {
	fmt.Println(aurora.Red("✖ " + text))   // stdout
}

Same for the token-refresh branch in checkErr (cmd/root.go:132, :136) and the bare fmt.Println(err) in Execute (cmd/root.go:107).

Every --json code path is preceded by a checkErr, and every printJSON call is itself wrapped in one. So:

$ apppack ps --json -a myapp | jq .
jq: parse error: Invalid numeric literal at line 1, column 3

...on an expired token, a missing app, or any AWS error. The ANSI-colored ✖ ... lands in jq's stdin, and the pipeline's exit code is jq's, not apppack's — so scripts don't even reliably detect the failure.

Worth noting #137's own commit message claims "errors still go to stderr with a non-zero exit code." That was the intent; it isn't what the code does.

printError → os.Stderr is a one-line fix, but it changes behavior CLI-wide (not just under --json), which is why I'm filing rather than just shipping it. Should errors move to stderr unconditionally, or only when AsJSON is set? Unconditionally is more correct and more conventional; it's also a visible change for anyone redirecting stdout today.

2. reviewapps --json --account X prepends a warning to the JSON

cmd/reviewapps.go:52-56, called at :66 — seven lines before the if AsJSON return at :73:

fmt.Println(aurora.Yellow("Warning: The 'account' flag is ignored for reviewapps..."))

$ apppack reviewapps my-pipeline --account acme --json | jq

gets a yellow prose line before the array. This is the only unconditional stray stdout write inside a --json path. Fix: move the call below the AsJSON return, or route it to stderr.

3. --json --debug interleaves logrus into the JSON document

cmd/root.go:59

if debug {
	logrus.SetOutput(os.Stdout)

Both are persistent root flags and nothing makes them mutually aware, so apppack build list --json --debug | jq is guaranteed to fail. os.Stderr is the fix, with no loss of function. Pre-existing line, but --json is what turned it into a bug.

4. build status --json emits "arns": null, which coerceNilSlice was written to prevent

cmd/json.go:22 only inspects the top-level value's kind:

func coerceNilSlice(v any) any {
	rv := reflect.ValueOf(v)
	if rv.Kind() == reflect.Slice && rv.IsNil() { ... }

app/builds.go:25 has Arns []string with no omitempty, and buildStatusJSON embeds six BuildPhaseDetail values (Build, Test, Finalize, Release, Postdeploy, Deploy). Any phase that hasn't run has Arns == nil, so:

$ apppack build status --json -a myapp | jq '.test.arns | length'
jq: error: null (null) has no length

Both build.go:881 (slice of structs — outer coerced, inner not) and build.go:916 (single struct — coercion is a no-op) are affected. Fix is either omitempty on the field or making coerceNilSlice recurse. omitempty changes the schema; recursing keeps the documented []-not-null promise. I'd lean recursive, but it's a schema decision.

Audited the other printJSON call sites — access.go:149, admins.go:77, auth.go:149, reviewapps.go:78, scheduledTasks.go:81, ps.go:154, stacks.go:97, version.go:56 are all clean (top-level slices, or built with make(..., 0, n) so non-nil by construction).

5. ps --json silently drops malformed tasks

cmd/ps.go:146 logs skipped tasks at Warn, but cmd/root.go:63 sets the level to ErrorLevel on every non---debug run, so it never emits. #137's commit message says these skips are surfaced "as logrus.Warn (not Debug) when --json is active" — they aren't. A task missing apppack:processType/apppack:buildNumber just vanishes from the array with no indication. (And when it does fire, under --debug, it goes to stdout per item 3.)

6. Spinners aren't gated on AsJSON — currently saved by accident

#137's commit message says "when --json is set the spinner is suppressed." There is no AsJSON check anywhere near spinner code; all 11 --json-capable commands call ui.StartSpinner() unconditionally before the AsJSON branch.

It's harmless today because ui.StartSpinner (ui/formatter.go:16) checks isatty.IsTerminal(os.Stdout.Fd()), and the spinner library re-checks independently — so | jq and > out.json are both clean. But under a pty (CI harnesses, script, docker -t, expect-style automation) isatty returns true and the cursor hide/show escapes wrap the JSON. Correctness rests on an unrelated isatty check rather than on the flag. An if AsJSON { return } guard at the top of StartSpinner would make it intentional.

7. --json is accepted everywhere but implemented on 11 commands

Implemented: config list, version, auth apps, reviewapps, access, ps, admins, stacks, scheduled-tasks, build list, build status.

Emits structured data, accepts --json, silently ignores it — notable ones:

• auth accounts (cmd/auth.go:198) — builds a 3-column tabwriter table, sitting right next to auth apps which did get JSON. The most glaring omission.
• events <service> (cmd/events.go:35) — timestamped event list, obvious candidate.
• config export (cmd/config.go:161) — already emits JSON, but via its own path (app.toJSON), ignoring --json and bypassing printJSON.
• version check (cmd/version.go:70) — while plain version got it.
• auth whoami, config get, ps resize|scale|restart, logs open, build start|watch, and all of create/modify/destroy/upgrade/db/shell.

A persistent flag that's silently ignored on most commands is a worse contract than one that errors. Options: gate the flag onto the subcommands that implement it, or have unsupported paths fail loudly. Either is a design call.

Also related: --non-interactive is not global (it's local to createCmd, cmd/create.go:325) and --json doesn't imply it. No --json path currently reaches a prompt, so there's no hang today — but scheduled-tasks delete --json accepts the flag, ignores it, and drops into an interactive huh select that will block a script forever.

Suggested order

1, 2, 3 are unambiguous bugs with one-line fixes and the widest blast radius. 4 and 5 are correctness. 6 is hardening. 7 is a contract decision worth its own discussion.

Happy to take 1–3 as a single PR once there's a call on the stdout-vs-stderr scope question in item 1.