From 28a3be88d20167e9ece63fb467d3cc686f451021 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 27 Jul 2026 21:03:28 -0700 Subject: [PATCH] Go: map HTTP 400 to validation (not api_error) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SPEC §6 prescribes 400 -> validation to match the other five SDKs, but Go's checkResponse had no 400 case, so it fell through to the default arm and mislabeled 400 Bad Request as api_error. Add the missing case (code=validation, http_status=400, retryable=false). This resolves the [CONFLICT]/[static] carve-outs the SPEC carried for 400: mark the error-code table row, the mapping-algorithm step, and the non-retryable-errors note as [conformance]-verified, and drop the stale "Go currently maps 400 to api_error" note. Adds a 400 case to the shared error-mapping conformance fixture (verified green across all five runners: Go, Kotlin, TypeScript, Ruby, Python) and a 400 row to helpers_test.go (proven to fail pre-fix: api_error != validation). retryable stays false — this is a code/message correction, not a retry-behavior change. Follows the retry-contract program (#456/#460/#461/#476). --- SPEC.md | 15 ++++++++------- conformance/tests/error-mapping.json | 17 +++++++++++++++++ go/pkg/basecamp/helpers.go | 2 ++ go/pkg/basecamp/helpers_test.go | 1 + 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/SPEC.md b/SPEC.md index 1e499604f..6b36fbd88 100644 --- a/SPEC.md +++ b/SPEC.md @@ -316,7 +316,7 @@ END ### Error Code Table -Status-mapped codes are verified per the Verification column. Most are `[conformance]`-verified; 400→`validation` is `[static]` (no conformance test). Client-side codes (`usage`, `network`, `ambiguous`) and exit codes are `[static]`. +Status-mapped codes are verified per the Verification column and are `[conformance]`-verified. Client-side codes (`usage`, `network`, `ambiguous`) and exit codes are `[static]`. | Code | Exit Code | HTTP Status | Retryable | Description | Verification | |------|-----------|-------------|-----------|-------------|-------------| @@ -329,11 +329,11 @@ Status-mapped codes are verified per the Verification column. Most are `[conform | `api_error` | 7 | 500, 502, 503, 504 | true | Server-side error | `[conformance]` | | `ambiguous` | 8 | — | false | Multiple matches found (CLI disambiguation) | `[static]` | | `validation` | 9 | 422 | false | Request validation failed | `[conformance]` | -| `validation` | 9 | 400 | false | Request validation failed | `[static]` | +| `validation` | 9 | 400 | false | Request validation failed | `[conformance]` | ### HTTP Status Mapping Algorithm -Each mapping below is `[conformance]`-verified except step 5 (400 → `validation`) which is `[static]`. +Each explicitly enumerated status mapping below (steps 1–10) is `[conformance]`-verified. The two catch-all fallback steps (11: general 5xx; 12: any other non-mapped status) have no dedicated conformance case and are `[static]`. Given an HTTP response with status code `status` and body `body`: @@ -341,14 +341,14 @@ Given an HTTP response with status code `status` and body `body`: 2. If `status == 403` → `BasecampError(code: "forbidden", http_status: 403, retryable: false)`. 3. If `status == 404` → `BasecampError(code: "not_found", http_status: 404, retryable: false)`. 4. If `status == 429` → `BasecampError(code: "rate_limit", http_status: 429, retryable: true, retry_after: parseRetryAfter(headers))`. -5. If `status == 400` → `BasecampError(code: "validation", http_status: 400, retryable: false)`. `[CONFLICT: Go currently maps 400 to "api_error" (falls through to default case). The spec prescribes "validation" to match other SDKs. No conformance test exists for 400 specifically.]` `[static]` +5. If `status == 400` → `BasecampError(code: "validation", http_status: 400, retryable: false)`. 6. If `status == 422` → `BasecampError(code: "validation", http_status: 422, retryable: false)`. 7. If `status == 500` → `BasecampError(code: "api_error", http_status: 500, retryable: true)`. 8. If `status == 502` → `BasecampError(code: "api_error", http_status: 502, retryable: true)`. 9. If `status == 503` → `BasecampError(code: "api_error", http_status: 503, retryable: true)`. 10. If `status == 504` → `BasecampError(code: "api_error", http_status: 504, retryable: true)`. -11. If `status >= 500` → `BasecampError(code: "api_error", http_status: status, retryable: true)`. -12. Otherwise → `BasecampError(code: "api_error", http_status: status, retryable: false)`. +11. If `status >= 500` → `BasecampError(code: "api_error", http_status: status, retryable: true)`. `[static]` +12. Otherwise → `BasecampError(code: "api_error", http_status: status, retryable: false)`. `[static]` In all cases, extract `request_id` from `X-Request-Id` response header if present. `[conformance]` @@ -772,7 +772,7 @@ All 4xx and 5xx responses must produce typed `BasecampError` errors (not silentl Status codes 401, 403, 404, and 422 must NOT be retried. Conformance tests assert `requestCount == 1` for these statuses. `[conformance]` -Status code 400 must also NOT be retried. This is a `[static]` contract (no dedicated conformance test exists for 400). +Status code 400 must also NOT be retried. The error-mapping conformance suite asserts `retryable == false` for 400 (mapped to `validation`), so its non-retryable classification is `[conformance]`-verified; there is no dedicated `requestCount` fixture for 400. `[conformance]` ### Retry Exhaustion @@ -1494,6 +1494,7 @@ account, attachments, automation, boosts, campfires, cardColumns, cardSteps, car | `error-mapping.json` | 401 → auth_required | §6 | | `error-mapping.json` | 403 → forbidden | §6 | | `error-mapping.json` | 404 → not_found | §6 | +| `error-mapping.json` | 400 → validation | §6 | | `error-mapping.json` | 422 → validation | §6 | | `error-mapping.json` | 429 → rate_limit | §6 | | `error-mapping.json` | 500 → api_error | §6 | diff --git a/conformance/tests/error-mapping.json b/conformance/tests/error-mapping.json index 395caee06..4e7e2683f 100644 --- a/conformance/tests/error-mapping.json +++ b/conformance/tests/error-mapping.json @@ -49,6 +49,23 @@ ], "tags": ["error-mapping", "not-found", "structured-error"] }, + { + "name": "400 maps to validation error code", + "description": "Verifies that 400 Bad Request maps to structured error with code 'validation' and retryable=false", + "operation": "GetProject", + "method": "GET", + "path": "/projects/{projectId}", + "pathParams": {"projectId": 12345}, + "mockResponses": [ + {"status": 400, "headers": {"X-Request-Id": "req-bad-400"}, "body": {"error": "Bad request"}} + ], + "assertions": [ + {"type": "errorCode", "expected": "validation"}, + {"type": "errorField", "path": "httpStatus", "expected": 400}, + {"type": "errorField", "path": "retryable", "expected": false} + ], + "tags": ["error-mapping", "validation", "structured-error"] + }, { "name": "422 maps to validation error code", "description": "Verifies that 422 Unprocessable Entity maps to structured error with code 'validation'", diff --git a/go/pkg/basecamp/helpers.go b/go/pkg/basecamp/helpers.go index 93ab836ff..6de8d503e 100644 --- a/go/pkg/basecamp/helpers.go +++ b/go/pkg/basecamp/helpers.go @@ -82,6 +82,8 @@ func checkResponse(resp *http.Response, body []byte) error { serverMsg, serverHint := parseErrorBody(body) switch resp.StatusCode { + case http.StatusBadRequest: + return &Error{Code: CodeValidation, Message: msgOrDefault(serverMsg, "validation error"), Hint: serverHint, HTTPStatus: 400, RequestID: requestID} case http.StatusUnauthorized: return &Error{Code: CodeAuth, Message: msgOrDefault(serverMsg, "authentication required"), Hint: serverHint, HTTPStatus: 401, RequestID: requestID} case http.StatusForbidden: diff --git a/go/pkg/basecamp/helpers_test.go b/go/pkg/basecamp/helpers_test.go index d1f82efb0..0ff0e9ee5 100644 --- a/go/pkg/basecamp/helpers_test.go +++ b/go/pkg/basecamp/helpers_test.go @@ -27,6 +27,7 @@ func TestCheckResponse_ErrorCodes(t *testing.T) { wantCode string wantRetry bool }{ + {400, CodeValidation, false}, {401, CodeAuth, false}, {403, CodeForbidden, false}, {404, CodeNotFound, false},