fix: Surface the error field and request ID on 422 exceptions - #1656
fix: Surface the error field and request ID on 422 exceptions#1656mjdavidson wants to merge 3 commits into
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const { status, data, headers } = response; | ||
|
|
||
| const requestID = headers['X-Request-ID'] ?? ''; | ||
| const requestID = headers.get('X-Request-ID') ?? ''; |
There was a problem hiding this comment.
🔴 Request timeouts crash with a TypeError instead of reporting the timeout error
The request-ID header is read using a method call (headers.get('X-Request-ID') at src/workos.ts:400) that only works on native Headers objects, but the timeout error path supplies a plain empty object instead, so any timed-out request throws an unrelated TypeError.
Impact: When any non-retryable API call times out, the SDK crashes with "headers.get is not a function" instead of surfacing the timeout error.
Timeout error path constructs headers as a plain object
In src/common/net/fetch-client.ts:306-313, when a request times out (AbortError), an HttpClientError is thrown with headers: {} — a plain JavaScript object. This error reaches handleHttpError in src/workos.ts:390, where headers.get('X-Request-ID') is called at line 400. Since {} doesn't have a .get() method, this throws TypeError: headers.get is not a function.
The old code used bracket access (headers['X-Request-ID']), which returns undefined on both plain objects and Headers objects, safely falling back to '' via ?? ''.
The fix should either:
- Make the timeout path in
fetch-client.ts:310use a realHeadersobject:headers: new Headers(), or - Use a defensive check in
workos.ts:400:typeof headers.get === 'function' ? headers.get('X-Request-ID') : headers['X-Request-ID']
Prompt for agents
The change from headers['X-Request-ID'] to headers.get('X-Request-ID') at src/workos.ts:400 breaks when the headers object is a plain JS object (not a native Headers instance). This happens in the timeout error path at src/common/net/fetch-client.ts:306-313, where HttpClientError is constructed with headers: {} (a plain object without a .get() method).
Two possible fixes:
1. In src/common/net/fetch-client.ts:310, change headers: {} to headers: new Headers() so it always has a .get() method.
2. In src/workos.ts:400, add a defensive check like: const requestID = (typeof headers.get === 'function' ? headers.get('X-Request-ID') : headers['X-Request-ID']) ?? '';
Option 1 is cleaner because it fixes the root cause — the timeout path should produce a proper Headers object consistent with the normal error path.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch, and confirmed. I reproduced it independently: the AbortError/timeout branch in fetch-client.ts constructs HttpClientError with headers: {}, so once handleHttpError reads the request ID via headers.get('X-Request-ID') the plain object throws TypeError: headers.get is not a function, masking the real timeout error.
Fixed via option 1 — the timeout path now constructs the error with new Headers() (commit a7f1eb1). I chose fixing it at the source over a defensive typeof headers.get === 'function' check in handleHttpError because it keeps a single, uniform header-access path (.get) — the same one already used for Retry-After on 429s — and guarantees every HttpClientError.response.headers is a fetch Headers instance rather than papering over an inconsistent shape at the read site.
Added a WorkOS-level regression test (when the request times out) that drives a timed-out request through WorkOS.post and asserts it rejects with the 408 timeout error (requestID: '') instead of a TypeError. Verified it fails on the pre-fix code with exactly headers.get is not a function and passes after.
| const { status, data, headers } = response; | ||
|
|
||
| const requestID = headers['X-Request-ID'] ?? ''; | ||
| const requestID = headers.get('X-Request-ID') ?? ''; |
There was a problem hiding this comment.
🔍 The bracket-to-.get() change also fixes a pre-existing silent bug for real API errors
The old code headers['X-Request-ID'] at src/workos.ts:400 was silently broken for normal (non-timeout) API errors too. In src/common/net/fetch-client.ts:292, the HttpClientError is constructed with headers: res.headers — a native Fetch API Headers object. Native Headers objects do not support bracket notation for reading header values (headers['X-Request-ID'] returns undefined), so the requestID was always '' for all error responses. The new .get() call fixes this, meaning error exceptions will now correctly include the X-Request-ID value. This is a positive behavioral change but could subtly affect error message formatting or logging downstream if anything depended on requestID being empty.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct, and this is intended. Before this change requestID was silently '' on every error thrown from handleHttpError, because headers is a fetch Headers instance (bracket access on it always returns undefined) — surfacing the real request ID is a primary goal of this PR, since support relies on it. The error-only 422 test asserting requestID: 'a-request-id' locks that in (the existing 404 tests use toStrictEqual on Error objects, which ignores custom fields, so they never caught the empty value).
On the downstream-formatting concern: requestID isn't interpolated into any exception message (message is derived from message/error/errors only), so this only populates the previously-empty requestID field. No behavior keyed on it being empty.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The timeout (408) path in FetchHttpClient constructed HttpClientError with a plain-object headers ({}). After switching handleHttpError to read the request ID with headers.get('X-Request-ID'), that path threw TypeError: headers.get is not a function, masking the timeout with an unrelated crash.
Construct the timeout error with new Headers() so every HttpClientError carries a fetch Headers instance and handleHttpError can read it uniformly. Add a WorkOS-level regression test asserting a timed-out request surfaces the 408 timeout error (not a TypeError).
Co-Authored-By: bot_apk <apk@cognition.ai>
|
Thanks to both reviewers. The one runtime bug both flagged — the timeout (408) path passing |
handleHttpError reads the request ID with headers.get(), so every HttpClientError must carry a fetch Headers instance. The headers field was typed any, which is how the timeout path compiled while passing a plain object and crashed at runtime. Typing it as Headers makes the compiler enforce the invariant at every construction site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Description
Some WorkOS API 422 responses carry only an
errorfield (for example, Vault's{"error": "Maximum unique keys reached"}).UnprocessableEntityExceptionignored that field, so applications saw only the genericUnprocessable entitymessage and had to reproduce the request with raw HTTP to learn the actual reason.ConflictException(409) already forwardserror— this brings the 422 path in line with it. Precedence is unchanged:messagewins overerror, and a structurederrorsarray still takes priority over both.Also fixes
requestIDbeing empty on every exception thrown fromhandleHttpError: it read the request ID with bracket access (headers['X-Request-ID']) on a fetchHeadersinstance, which always returnsundefined— the same function already usesheaders.get('Retry-After')correctly for 429s. Support relies on request IDs, so exceptions now carry them.Tests cover the new 422 cases (error-only body, message precedence, errors-array precedence) — the error-only case also locks in the
requestIDfix, since it asserts the value that bracket access silently dropped.Documentation
Devin shepherd: https://app.devin.ai/sessions/bbb48b72a3544efaa196436ee81ac731