From 06ba912883892611b17fdd0f490fc0aa27a3f5d1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 16:24:46 -0700 Subject: [PATCH 1/3] Emit retryable in the JSON error envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- README.md | 19 +++++ e2e/errors.bats | 81 ++++++++++++++++++++ internal/commands/projects_test.go | 40 ++++++++++ internal/output/envelope.go | 28 ++++--- internal/output/output_test.go | 115 +++++++++++++++++++++++++++++ skills/basecamp/SKILL.md | 12 ++- 6 files changed, 283 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 46567c54b..8865bd339 100644 --- a/README.md +++ b/README.md @@ -134,6 +134,25 @@ Every command supports `--json` for structured output: Breadcrumbs suggest next commands, making it easy for humans and agents to navigate. +Errors use the same envelope with `ok: false`, a stable `code`, and `retryable`, +which says whether a retry can change the outcome: + +```json +{ + "ok": false, + "error": "Gateway error (503)", + "code": "api_error", + "retryable": true, + "hint": "..." +} +``` + +`retryable` is present on every error envelope — `true` for transient failures +(network, timeout, rate limit, 5xx/gateway, circuit open), `false` for a verdict +(usage, not found, auth, forbidden, validation, account limit) — and never on a +success envelope. Key on it rather than on the code or message when deciding +whether to retry. + ## Authentication OAuth 2.1 with automatic token refresh. First login opens your browser. diff --git a/e2e/errors.bats b/e2e/errors.bats index 56b768aea..63e8b288b 100644 --- a/e2e/errors.bats +++ b/e2e/errors.bats @@ -481,5 +481,86 @@ load test_helper assert_failure assert_json_value '.ok' 'false' assert_json_value '.code' 'usage' + assert_json_value '.retryable' 'false' assert_output_contains '"error"' } + +# Every error envelope carries "retryable" so a consumer can tell a transient +# failure from a verdict without parsing the code or message. A stub API that +# answers 503 exercises the SDK classification end to end; a mutation is used +# because the SDK retries idempotent reads on its own schedule. + +start_unavailable_api_stub() { + UNAVAILABLE_STUB_PORT_FILE="$TEST_TEMP_DIR/unavailable-stub.port" + + local python_bin + if command -v python3 >/dev/null 2>&1; then + python_bin=python3 + elif command -v python >/dev/null 2>&1; then + python_bin=python + else + echo "Error: neither python3 nor python is available in PATH; cannot start unavailable API stub" >&2 + return 1 + fi + + "$python_bin" - <<'PY' "$UNAVAILABLE_STUB_PORT_FILE" & +import http.server +import socketserver +import sys + +port_file = sys.argv[1] + +class Handler(http.server.BaseHTTPRequestHandler): + def _unavailable(self): + self.send_response(503) + self.send_header('Content-Type', 'application/json') + self.end_headers() + self.wfile.write(b'{"error":"service unavailable"}') + + do_GET = _unavailable + do_POST = _unavailable + + def log_message(self, format, *args): + pass + +with socketserver.TCPServer(('127.0.0.1', 0), Handler) as server: + with open(port_file, 'w', encoding='utf-8') as f: + f.write(str(server.server_address[1])) + server.serve_forever() +PY + UNAVAILABLE_STUB_PID=$! + + for _ in $(seq 1 50); do + [[ -s "$UNAVAILABLE_STUB_PORT_FILE" ]] && break + sleep 0.1 + done + + if [[ ! -s "$UNAVAILABLE_STUB_PORT_FILE" ]]; then + echo "failed to start unavailable API stub" >&2 + return 1 + fi + + export BASECAMP_BASE_URL="http://127.0.0.1:$(cat "$UNAVAILABLE_STUB_PORT_FILE")" +} + +stop_unavailable_api_stub() { + if [[ -n "${UNAVAILABLE_STUB_PID:-}" ]]; then + kill "$UNAVAILABLE_STUB_PID" 2>/dev/null || true + wait "$UNAVAILABLE_STUB_PID" 2>/dev/null || true + unset UNAVAILABLE_STUB_PID + fi +} + +@test "transient API failure returns retryable JSON envelope" { + start_unavailable_api_stub + create_credentials + create_global_config '{"account_id": 99999}' + + run basecamp projects create "Launch plan" --json + stop_unavailable_api_stub + + assert_failure + assert_json_value '.ok' 'false' + assert_json_value '.code' 'api_error' + assert_json_value '.retryable' 'true' +} diff --git a/internal/commands/projects_test.go b/internal/commands/projects_test.go index 00e188e42..557720f3f 100644 --- a/internal/commands/projects_test.go +++ b/internal/commands/projects_test.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "net/http/httptest" "strings" "testing" @@ -150,3 +151,42 @@ type projectUpdateEnvelope struct { UpdatedAt string `json:"updated_at"` } `json:"data"` } + +// A failed command reaches the JSON error envelope with a top-level +// "retryable" that carries the SDK's classification end to end (SDK error → +// convertSDKError → app.Err): a transient 503 says retry, a 404 verdict says +// don't. Consumers deciding whether to retry key on that field rather than +// on the code or message. A mutation is used because the generated client +// retries idempotent operations on its own schedule, which is not the +// behavior under test here. +func TestProjectsCreateErrorEnvelopeCarriesRetryable(t *testing.T) { + for _, tc := range []struct { + name string + status int + code string + retryable bool + }{ + {"gateway 503", http.StatusServiceUnavailable, basecamp.CodeAPI, true}, + {"not found 404", http.StatusNotFound, basecamp.CodeNotFound, false}, + } { + t.Run(tc.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(tc.status) + fmt.Fprint(w, `{"error":"upstream said no"}`) + })) + t.Cleanup(server.Close) + app, buf := newRequestLevelApp(t, server.URL) + + err := executeCommand(NewProjectsCmd(), app, "create", "Launch plan") + require.Error(t, err) + require.NoError(t, app.Err(err)) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded), "envelope: %s", buf.String()) + assert.Equal(t, false, decoded["ok"]) + assert.Equal(t, tc.code, decoded["code"]) + assert.Equal(t, tc.retryable, decoded["retryable"], "envelope: %s", buf.String()) + }) + } +} diff --git a/internal/output/envelope.go b/internal/output/envelope.go index 59b83fb28..c3798cef2 100644 --- a/internal/output/envelope.go +++ b/internal/output/envelope.go @@ -55,12 +55,21 @@ type Breadcrumb struct { } // ErrorResponse is the error envelope for JSON output. +// +// Retryable is always present on an error envelope and never on a success +// one: true when the failure was classified transient (network, timeout, +// rate limit, 5xx/gateway, circuit open, bulkhead full), false for a verdict +// (usage, not found, auth, forbidden, validation, account limit) and for any +// error nothing classified. Consumers key on the field rather than on the +// code or message, which describe the failure and not whether a retry can +// change it. type ErrorResponse struct { - OK bool `json:"ok"` - Error string `json:"error"` - Code string `json:"code"` - Hint string `json:"hint,omitempty"` - Meta map[string]any `json:"meta,omitempty"` + OK bool `json:"ok"` + Error string `json:"error"` + Code string `json:"code"` + Retryable bool `json:"retryable"` + Hint string `json:"hint,omitempty"` + Meta map[string]any `json:"meta,omitempty"` } // Format specifies the output format. @@ -156,10 +165,11 @@ func (w *Writer) OK(data any, opts ...ResponseOption) error { func (w *Writer) Err(err error, opts ...ErrorResponseOption) error { e := AsError(err) resp := &ErrorResponse{ - OK: false, - Error: e.Message, - Code: e.Code, - Hint: e.Hint, + OK: false, + Error: e.Message, + Code: e.Code, + Retryable: e.Retryable, + Hint: e.Hint, } if requestID := RequestID(err); requestID != "" { if resp.Meta == nil { diff --git a/internal/output/output_test.go b/internal/output/output_test.go index a79485f06..5177432a7 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -470,6 +470,121 @@ func TestWriterErrIncludesRequestIDMeta(t *testing.T) { assert.Equal(t, "req-cli-123", resp.Meta["request_id"]) } +// The error envelope carries a top-level "retryable" that mirrors the +// error's Retryable classification. A consumer deciding whether to retry keys +// on it, not on the code or message — so it must be present (as false) on +// every error envelope, not just the transient ones, and never leak onto a +// success envelope. +func TestWriterErrEmitsRetryableForTransientError(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatJSON, Writer: &buf}) + + err := w.Err(&basecamp.Error{ + Code: basecamp.CodeAPI, + Message: "Gateway error (503)", + HTTPStatus: 503, + Retryable: true, + }) + require.NoError(t, err, "Err() failed") + + decoded := decodeErrorEnvelope(t, buf.Bytes()) + assert.Equal(t, false, decoded["ok"]) + assert.Equal(t, CodeAPI, decoded["code"]) + assert.Equal(t, true, decoded["retryable"]) +} + +func TestWriterErrEmitsRetryableFalseForVerdict(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatJSON, Writer: &buf}) + + require.NoError(t, w.Err(ErrNotFound("project", "123"))) + + decoded := decodeErrorEnvelope(t, buf.Bytes()) + retryable, present := decoded["retryable"] + assert.True(t, present, "retryable must be present on every error envelope, false included") + assert.Equal(t, false, retryable) +} + +func TestWriterErrRetryableForCLIClassifiedErrors(t *testing.T) { + for name, err := range map[string]error{ + "rate limit": ErrRateLimit(30), + "network": ErrNetwork(errors.New("dial tcp: connection refused")), + "wrapped": fmt.Errorf("listing projects: %w", ErrNetwork(errors.New("timeout"))), + } { + t.Run(name, func(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatJSON, Writer: &buf}) + + require.NoError(t, w.Err(err)) + + assert.Equal(t, true, decodeErrorEnvelope(t, buf.Bytes())["retryable"]) + }) + } +} + +func TestWriterErrRetryableFalseForUnclassifiedError(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatJSON, Writer: &buf}) + + require.NoError(t, w.Err(errors.New("something unexpected"))) + + decoded := decodeErrorEnvelope(t, buf.Bytes()) + assert.Equal(t, CodeAPI, decoded["code"]) + assert.Equal(t, false, decoded["retryable"], "no positive signal means not retryable") +} + +// Field order is part of the envelope contract: retryable sits between code +// and hint, and the existing fields keep their positions. +func TestWriterErrRetryableFieldOrder(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatJSON, Writer: &buf}) + + require.NoError(t, w.Err(ErrRateLimit(30))) + + out := buf.String() + positions := []int{ + strings.Index(out, `"ok"`), + strings.Index(out, `"error"`), + strings.Index(out, `"code"`), + strings.Index(out, `"retryable"`), + strings.Index(out, `"hint"`), + } + for i, pos := range positions { + require.NotEqual(t, -1, pos, "field %d missing from %s", i, out) + if i > 0 { + assert.Greater(t, pos, positions[i-1], "field order drifted in %s", out) + } + } +} + +func TestWriterErrQuietModeCarriesRetryable(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatQuiet, Writer: &buf}) + + require.NoError(t, w.Err(ErrRateLimit(30))) + + assert.Equal(t, true, decodeErrorEnvelope(t, buf.Bytes())["retryable"]) +} + +func TestWriterOKOmitsRetryable(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatJSON, Writer: &buf}) + + require.NoError(t, w.OK(map[string]string{"id": "123"})) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded)) + _, present := decoded["retryable"] + assert.False(t, present, "retryable is an error-envelope field only") +} + +func decodeErrorEnvelope(t *testing.T, raw []byte) map[string]any { + t.Helper() + var decoded map[string]any + require.NoError(t, json.Unmarshal(raw, &decoded), "Failed to unmarshal %s", raw) + return decoded +} + func TestWriterQuietFormat(t *testing.T) { var buf bytes.Buffer w := New(Options{ diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index d41407960..f71a8fa64 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -147,7 +147,7 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, |------|------|--------| | Filter/extract JSON data | `--jq ''` | Built-in jq filter (no external jq needed). Implies `--json`; filter runs on the envelope. | | Filter in agent mode | `--agent --jq ''` | Filter runs on data-only payload (no envelope), matching `--agent` contract. | -| Full JSON output | `--json` | JSON envelope: `{ok, data, summary, breadcrumbs, meta}` | +| Full JSON output | `--json` | JSON envelope: `{ok, data, summary, breadcrumbs, meta}`; errors: `{ok:false, error, code, retryable, hint, meta}` | | Show results to a user | `--md` / `-m` | GFM tables, task lists, structured Markdown | | Automation / scripting | `--agent` | Success: raw JSON data (no envelope); errors: `{ok:false,...}` object; no interactive prompts | @@ -1349,15 +1349,21 @@ the specific argument. Use this for elicitation: ```bash $ basecamp todos create --json -{"ok": false, "error": " required", "code": "usage", +{"ok": false, "error": " required", "code": "usage", "retryable": false, "hint": "Usage: basecamp todos create "} $ basecamp comments create 123 --json -{"ok": false, "error": " required", "code": "usage", ...} +{"ok": false, "error": " required", "code": "usage", "retryable": false, ...} ``` The `error` field names the missing `` — use it to prompt the user for the specific value. +**Retryable errors (`retryable`):** every error envelope carries a boolean `retryable` — +`true` when the failure is transient (network, timeout, rate limit, 5xx/gateway, circuit +open) and a retry can change the outcome, `false` for a verdict (usage, not found, auth, +forbidden, validation, account limit). Key on it rather than on `code` or `error` when +deciding whether to retry; it is never present on a success envelope. + **URL malformed (curl exit 3):** Special characters in content. Use plain text or properly escaped HTML. ## Built-in jq Filtering From 08b86a50e3a0f6c4b1dea6bfdd6e74dc8a062492 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 16:58:37 -0700 Subject: [PATCH 2/3] Converge the review round: honest retryable docs, no leaked test stub MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- README.md | 12 +++++++----- e2e/errors.bats | 7 +++++-- internal/output/envelope.go | 3 ++- internal/output/output_test.go | 8 ++++++-- skills/basecamp/SKILL.md | 11 +++++++---- 5 files changed, 27 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 8865bd339..0a94548c4 100644 --- a/README.md +++ b/README.md @@ -147,11 +147,13 @@ which says whether a retry can change the outcome: } ``` -`retryable` is present on every error envelope — `true` for transient failures -(network, timeout, rate limit, 5xx/gateway, circuit open), `false` for a verdict -(usage, not found, auth, forbidden, validation, account limit) — and never on a -success envelope. Key on it rather than on the code or message when deciding -whether to retry. +`retryable` is present on every error envelope — `true` when the CLI classified +the failure transient (network, timeout, rate limit, circuit open, and most +5xx/gateway responses — not all: 507 and some 500s are verdicts), `false` for a +verdict (usage, not found, auth, forbidden, validation, account limit) and for +any error nothing classified — and never on a success envelope. Key on it rather +than on the code or message when deciding whether to retry; `false` means no +known reason a retry would help, not a guarantee the failure is permanent. ## Authentication diff --git a/e2e/errors.bats b/e2e/errors.bats index 63e8b288b..cbb828a79 100644 --- a/e2e/errors.bats +++ b/e2e/errors.bats @@ -492,6 +492,7 @@ load test_helper start_unavailable_api_stub() { UNAVAILABLE_STUB_PORT_FILE="$TEST_TEMP_DIR/unavailable-stub.port" + local log_file="$TEST_TEMP_DIR/unavailable-stub.log" local python_bin if command -v python3 >/dev/null 2>&1; then @@ -503,7 +504,7 @@ start_unavailable_api_stub() { return 1 fi - "$python_bin" - <<'PY' "$UNAVAILABLE_STUB_PORT_FILE" & + "$python_bin" - <<'PY' "$UNAVAILABLE_STUB_PORT_FILE" >"$log_file" 2>&1 3>&- & import http.server import socketserver import sys @@ -536,7 +537,9 @@ PY done if [[ ! -s "$UNAVAILABLE_STUB_PORT_FILE" ]]; then - echo "failed to start unavailable API stub" >&2 + echo "failed to start unavailable API stub. Log:" >&2 + cat "$log_file" >&2 + stop_unavailable_api_stub return 1 fi diff --git a/internal/output/envelope.go b/internal/output/envelope.go index c3798cef2..6903cf8df 100644 --- a/internal/output/envelope.go +++ b/internal/output/envelope.go @@ -58,7 +58,8 @@ type Breadcrumb struct { // // Retryable is always present on an error envelope and never on a success // one: true when the failure was classified transient (network, timeout, -// rate limit, 5xx/gateway, circuit open, bulkhead full), false for a verdict +// rate limit, circuit open, bulkhead full, and most 5xx/gateway responses — +// not all: 507 and a raw-client 500 are verdicts), false for a verdict // (usage, not found, auth, forbidden, validation, account limit) and for any // error nothing classified. Consumers key on the field rather than on the // code or message, which describe the failure and not whether a retry can diff --git a/internal/output/output_test.go b/internal/output/output_test.go index 5177432a7..959b20aec 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -533,8 +533,12 @@ func TestWriterErrRetryableFalseForUnclassifiedError(t *testing.T) { assert.Equal(t, false, decoded["retryable"], "no positive signal means not retryable") } -// Field order is part of the envelope contract: retryable sits between code -// and hint, and the existing fields keep their positions. +// Field order is part of the piped-envelope contract: retryable sits between +// code and hint, and the existing fields keep their positions. The pin covers +// the non-TTY representation machine consumers read; on a TTY, writeJSON +// re-encodes the envelope through map-backed sanitization, so keys come out +// alphabetized there — that path serves a human reader and carries no order +// contract. func TestWriterErrRetryableFieldOrder(t *testing.T) { var buf bytes.Buffer w := New(Options{Format: FormatJSON, Writer: &buf}) diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index f71a8fa64..ca867626c 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1359,10 +1359,13 @@ $ basecamp comments create 123 --json The `error` field names the missing `` — use it to prompt the user for the specific value. **Retryable errors (`retryable`):** every error envelope carries a boolean `retryable` — -`true` when the failure is transient (network, timeout, rate limit, 5xx/gateway, circuit -open) and a retry can change the outcome, `false` for a verdict (usage, not found, auth, -forbidden, validation, account limit). Key on it rather than on `code` or `error` when -deciding whether to retry; it is never present on a success envelope. +`true` when the CLI classified the failure transient (network, timeout, rate limit, +circuit open, most 5xx/gateway responses — not all: 507 and some 500s are verdicts) and +a retry can change the outcome, `false` for a verdict (usage, not found, auth, forbidden, +validation, account limit) and for any error nothing classified. Key on it rather than +on `code` or `error` when deciding whether to retry — `false` means no known reason a +retry would help, not a guarantee of permanence; it is never present on a success +envelope. **URL malformed (curl exit 3):** Special characters in content. Use plain text or properly escaped HTML. From e66687d0e87e56e7c82ed43e944cdfb5c5d0bc8e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 17:13:59 -0700 Subject: [PATCH 3/3] Say the envelope contract is keys, not key order 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. --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 0a94548c4..59c8b4d2b 100644 --- a/README.md +++ b/README.md @@ -147,6 +147,10 @@ which says whether a retry can change the outcome: } ``` +Key order within the envelope is not part of the contract — the interactive +(TTY) path re-encodes through a map and alphabetizes keys — so match on key +names, never on position. + `retryable` is present on every error envelope — `true` when the CLI classified the failure transient (network, timeout, rate limit, circuit open, and most 5xx/gateway responses — not all: 507 and some 500s are verdicts), `false` for a