Emit retryable in the JSON error envelope

Merged
JE
2mo ago
+300-12

What

Add a top-level boolean retryable to the --json error envelope, mirroring the error's Retryable classification: always present on error envelopes, never on success ones.

Why

The SDK and the CLI both classify failures as retryable, and output.Error carries that flag (internal/output/errors.go:49 copies sdkErr.Retryable), but the --json error envelope dropped it. A consumer of the envelope could not tell a transient failure from a verdict without keeping its own list of codes and message spellings — which is exactly what basecamp-local-agent-connector had to do (TRANSIENT_CODES + a message regex), and which drifts the moment either side changes wording.

Contract

The error envelope carries a top-level boolean retryable:

{"ok": false, "error": "Gateway error (503)", "code": "api_error", "retryable": true, "hint": "..."}

Contract:

  • Always present on an error envelope, false when nothing classified the error as retryable (no positive signal ⇒ not retryable).
  • Never present on a success envelope.
  • Field order: ok, error, code, retryable, hint, meta — existing fields keep their positions; the field is additive.
  • Applies to every sink that emits the error object: --json, --agent/--quiet ({ok:false,...}), and --jq (filter runs on the envelope). Styled/markdown renderers are unchanged.

basecamp-local-agent-connector will key on this field when present and fall back to its list only for older CLIs that omit it.

Where Retryable is set (audit)

true:

  • SDK checkResponse (generated service layer, helpers.go): 429; every 5xx except 507.
  • SDK singleRequest (raw client): 429 (ErrRateLimit); 502/503/504 gateway errors; http.Client.Do failure → ErrNetwork (covers DNS, connection refused, TLS, and client/context timeouts).
  • SDK ErrRateLimit, ErrNetwork constructors; oauth/discovery.go and oauth/device_errors.go network failures.
  • SDK internal "Token refreshed" marker after a 401 refresh (consumed by the retry loop).
  • Shared basecamp/cli output.ErrRateLimit, output.ErrNetwork.
  • CLI convertSDKError (internal/commands/projects.go, internal/names/resolver.go): ErrRateLimited, ErrCircuitOpen, ErrBulkheadFull; SDK errors pass their flag through.
  • Batch commands (comment.go, todos.go, todolists.go, assign.go) preserve outErr.Retryable when re-wrapping the first API error.

false (explicit or by default): usage, not found, auth, forbidden, validation (400/422), 507 account limit, raw-client 500 (ErrAPI(500, …) — the raw client deliberately does not retry 500; the generated service layer does mark 500 retryable, so retryable on a 500 depends on which path the command took, mirroring the SDK's own retry behaviour), and anything unclassified (AsError fallback).

Testing

  • make check passes (run as bin/ci on a 32-core builder; see below)

  • internal/output: retryable SDK error → true; ErrNotFound → key present and false; CLI ErrRateLimit/ErrNetwork/wrapped → true; unclassified error → false; field-order pin; quiet-mode envelope carries it; success envelope omits it.

  • internal/commands: projects create against an httptest server answering 503 → retryable: true, 404 → false (SDK → convertSDKError → app.Err). A mutation is used because the generated client retries idempotent reads on its own 1s/2s schedule, which is not under test here.

  • e2e/errors.bats: the usage-envelope test asserts .retryable == false; a new test runs the real binary against a python stub answering 503 and asserts .retryable == true, .code == api_error.

All of these fail on main (checked by stashing envelope.go): the Go tests fail on the missing key, the bats tests see null.

Declined alternatives

  • omitempty on retryable — would make false indistinguishable from "old CLI, field unknown", which is the exact ambiguity the connector's fallback exists to resolve. Always-present on errors is the contract.

  • Put it under meta — meta is for request-scoped diagnostics (request_id, stats); whether to retry is a property of the error itself and belongs beside code.

  • Emit retryable: false on success envelopes for symmetry — meaningless on success and would change every success payload; consumers gate on ok first.

  • Also emit a retry_after — the SDK does not surface the parsed Retry-After on the error (it consumes it in its own backoff), so there is nothing truthful to put there yet; a follow-up if the SDK exposes it.

  • Reconcile the raw-client 500 vs service-layer 500 classification — that is SDK behaviour, and the envelope should report the SDK's verdict rather than second-guess it.

  • Preserve envelope field order on TTY JSON output (Codex P2) — the TTY path re-encodes through map-backed sanitization (#479) and alphabetizes keys for every envelope field, predating retryable. Machine consumers (the connector, scripts, --jq) read piped output, which keeps struct order; the TTY rendering serves a human whose parser is their eyes, and JSON consumers must not key on order anyway. The order-pin test comment now scopes the contract to the piped representation instead. Also declined pinning the alphabetical TTY order in a test — that would enshrine incidental behavior as contract.