diff --git a/README.md b/README.md index 46567c54..59c8b4d2 100644 --- a/README.md +++ b/README.md @@ -134,6 +134,31 @@ 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": "..." +} +``` + +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 +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 OAuth 2.1 with automatic token refresh. First login opens your browser. diff --git a/e2e/errors.bats b/e2e/errors.bats index 56b768ae..cbb828a7 100644 --- a/e2e/errors.bats +++ b/e2e/errors.bats @@ -481,5 +481,89 @@ 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 log_file="$TEST_TEMP_DIR/unavailable-stub.log" + + 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" >"$log_file" 2>&1 3>&- & +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. Log:" >&2 + cat "$log_file" >&2 + stop_unavailable_api_stub + 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 00e188e4..557720f3 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 59b83fb2..6903cf8d 100644 --- a/internal/output/envelope.go +++ b/internal/output/envelope.go @@ -55,12 +55,22 @@ 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, 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 +// 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 +166,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 a79485f0..959b20ae 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -470,6 +470,125 @@ 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 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}) + + 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 d4140796..ca867626 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,24 @@ 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 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. ## Built-in jq Filtering