Emit retryable in the JSON error envelope - #665
Conversation
The SDK and the CLI both classify failures as retryable — 5xx/gateway, network, rate limit, circuit open, bulkhead full — and output.Error carries that flag, but the -j error envelope dropped it. A consumer reading the envelope could not tell a transient failure from a verdict without keeping its own list of codes and message spellings, which drifts from the CLI's classification the moment either side changes. The error envelope now carries a top-level boolean "retryable" between code and hint: true when the error was classified retryable, false otherwise, always present on error envelopes and never on success envelopes. The field is additive; existing fields keep their positions.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Pull request overview
Adds retry classification to JSON error envelopes for reliable automation.
Changes:
- Emits
retryableon error envelopes across JSON, agent, quiet, and jq paths. - Adds unit, command-level, and end-to-end coverage.
- Documents the new field, though the 5xx classification needs correction.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/output/envelope.go |
Adds and populates retryable. |
internal/output/output_test.go |
Tests envelope behavior and ordering. |
internal/commands/projects_test.go |
Tests SDK classification propagation. |
e2e/errors.bats |
Tests transient errors through the binary. |
README.md |
Documents the public contract. |
skills/basecamp/SKILL.md |
Updates agent guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06ba912883
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The README, SKILL.md, and the ErrorResponse doc comment overstated the classifier two ways. They described retryable:false as strictly a verdict, omitting the unclassified fallback — a consumer keying on the field per the docs would treat a transient failure the classification chain lost (a token-refresh network error surfacing as api_error, a raw-client 500) as permanent and never retry. And they taught that all 5xx/gateway responses are retryable when 507 and a raw-client 500 are verdicts, inviting retries that cannot succeed. All three sites now state the classification as a verdict with its exceptions and spell out that false means no known reason a retry would help, not proof of permanence. The field-order pin in output_test.go claimed the envelope contract unqualified, but on a TTY writeJSON re-encodes through map-backed sanitization and alphabetizes keys — behavior that predates the field and serves a human reader. The comment now scopes the pin to the piped representation machine consumers read. start_unavailable_api_stub's startup-timeout branch returned without killing the python stub it had spawned, leaving an orphaned listener whose inherited stdout/stderr/FD-3 could hold the bats run open past the failed test. The stub's output now goes to a log file (dumped on startup failure, matching the recorder helper) with FD 3 closed, and the timeout branch stops the stub before returning.
|
Since last push ( Applied
Declined
Tests: |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The interactive path re-encodes the envelope through a map and alphabetizes keys, so a consumer that reads positions breaks on a TTY. The contract is the key set; write that down.
What
Add a top-level boolean
retryableto the--jsonerror 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.Errorcarries that flag (internal/output/errors.go:49copiessdkErr.Retryable), but the--jsonerror 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 whatbasecamp-local-agent-connectorhad 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:
falsewhen nothing classified the error as retryable (no positive signal ⇒ not retryable).ok, error, code, retryable, hint, meta— existing fields keep their positions; the field is additive.--json,--agent/--quiet({ok:false,...}), and--jq(filter runs on the envelope). Styled/markdown renderers are unchanged.basecamp-local-agent-connectorwill key on this field when present and fall back to its list only for older CLIs that omit it.Where
Retryableis set (audit)true:checkResponse(generated service layer,helpers.go): 429; every 5xx except 507.singleRequest(raw client): 429 (ErrRateLimit); 502/503/504 gateway errors;http.Client.Dofailure →ErrNetwork(covers DNS, connection refused, TLS, and client/context timeouts).ErrRateLimit,ErrNetworkconstructors;oauth/discovery.goandoauth/device_errors.gonetwork failures.basecamp/clioutput.ErrRateLimit,output.ErrNetwork.convertSDKError(internal/commands/projects.go,internal/names/resolver.go):ErrRateLimited,ErrCircuitOpen,ErrBulkheadFull; SDK errors pass their flag through.comment.go,todos.go,todolists.go,assign.go) preserveoutErr.Retryablewhen 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, soretryableon a 500 depends on which path the command took, mirroring the SDK's own retry behaviour), and anything unclassified (AsErrorfallback).Testing
make checkpasses (run asbin/cion a 32-core builder; see below)internal/output: retryable SDK error →true;ErrNotFound→ key present andfalse; CLIErrRateLimit/ErrNetwork/wrapped →true; unclassified error →false; field-order pin; quiet-mode envelope carries it; success envelope omits it.internal/commands:projects createagainst 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 stashingenvelope.go): the Go tests fail on the missing key, the bats tests seenull.Declined alternatives
omitemptyonretryable— would makefalseindistinguishable 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—metais for request-scoped diagnostics (request_id,stats); whether to retry is a property of the error itself and belongs besidecode.Emit
retryable: falseon success envelopes for symmetry — meaningless on success and would change every success payload; consumers gate onokfirst.Also emit a
retry_after— the SDK does not surface the parsedRetry-Afteron 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 (security: strip ANSI/OSC escapes from API-controlled output #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.