feat: surface error metadata from API responses#369
Conversation
API error responses can include `code` and `error` fields beyond the message, but these were being discarded. Exposing them on exception objects lets consumers distinguish error types programmatically (e.g. rate-limit reason, validation code) without parsing the message string.
Greptile SummaryThis PR surfaces Confidence Score: 5/5Safe to merge — no logic errors, type-safety is maintained, and coverage is solid. All four changed files are internally consistent: decodeErrorBody() always returns both new keys, every exception constructor receives them, and is_string guards protect both entry points (fromResponse and the HTTP client path). No P0 or P1 issues found. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant API as WorkOS API
participant HC as HttpClient
participant DE as decodeErrorBody()
participant MA as mapApiException()
participant EX as Exception (ApiException subclass)
API->>HC: HTTP error response (4xx/5xx)
HC->>DE: ResponseInterface
DE->>DE: Parse body JSON
DE-->>HC: {message, code: ?string, error: ?string}
HC->>MA: response + body
MA->>MA: match statusCode
MA->>EX: new XxxException(message, statusCode, requestId, previous, code, error)
Note over EX: errorCode = code<br/>error = error<br/>retryAfter (429 only)
EX-->>HC: ApiException subclass
HC-->>API: throw exception
Reviews (3): Last reviewed commit: "test: Add coverage for errorCode and err..." | Re-trigger Greptile |
| $message = $decoded['message'] ?? $decoded['error_description'] ?? $decoded['error'] ?? null; | ||
| if (is_string($message) && $message !== '') { | ||
| return ['message' => $message]; | ||
| $code = isset($decoded['code']) && is_string($decoded['code']) ? $decoded['code'] : null; | ||
| $error = isset($decoded['error']) && is_string($decoded['error']) ? $decoded['error'] : null; | ||
| return ['message' => $message, 'code' => $code, 'error' => $error]; |
There was a problem hiding this comment.
error field populated when it also serves as message fallback
When a response contains only {"error": "invalid_grant"} (no message or error_description), the fallback at line 320 assigns $decoded['error'] to $message, and line 323 assigns the same value to $error. The resulting exception will have $exception->getMessage() === $exception->error, which offers no additional machine-readable signal over the message string. Callers expecting error to be a distinct, stable code may be misled.
There was a problem hiding this comment.
Fair observation, but fine to leave as-is. When the API returns only {"error": "invalid_grant"}, both getMessage() and ->error will be "invalid_grant". That's redundant but accurate — it faithfully reflects what the API sent. The machine-readable signal consumers should key on is errorCode (from the code field), and setting error to null in the fallback case would silently discard data the API actually returned.
`decodeErrorBody()` validates that `code` and `error` are strings before assigning, but `fromResponse()` did not. Under `strict_types=1`, a non-string value would throw a `TypeError`.
| ?\Throwable $previous = null, | ||
| ?string $errorCode = null, | ||
| ?string $error = null, | ||
| ?int $retryAfter = null, | ||
| ) { |
There was a problem hiding this comment.
Breaking constructor change —
$retryAfter shifted by two positions
$errorCode and $error are inserted before $retryAfter, changing its positional index from 5 to 7. Any caller that previously passed a $retryAfter value positionally — e.g. new RateLimitExceededException('msg', 429, $rid, null, 30) — will now silently pass 30 to $errorCode (?string). Under declare(strict_types=1) this throws a TypeError at runtime. Appending the new parameters after $retryAfter (or making them named-only via promotion at the end) would keep the signature backward-compatible.
There was a problem hiding this comment.
The only call site is internal (HttpClient::mapApiException()), which already passes args in the new order. No end user should be constructing SDK exception classes directly — they're thrown by the HTTP client, not instantiated by consumers. Not a practical concern.
The new properties added in feat/66fba13 had no test coverage. These tests exercise the full path through decodeErrorBody() and mapApiException() for each scenario: fields present, fields absent, empty body, rate-limit with Retry-After, and non-string values rejected by the is_string() guards.
Summary
errorCodeanderrorreadonly properties toApiExceptionand propagate them through
RateLimitExceededExceptionHttpClient::decodeErrorBody()to extractcodeanderrorfrom JSON error responses alongside the existingmessageHTTP status code match expression
This lets SDK consumers distinguish error types programmatically
(e.g. validation codes, rate-limit reasons) without parsing the
human-readable message string.
Test plan
errorCodeanderrorwhen APIreturns them in the response body
errorCodeanderrorarenullwhen the APIresponse omits those fields
RateLimitExceededExceptionstill correctly capturesthe
Retry-Afterheader alongside the new fields