Skip to content

fix(commons): read the HTTP error response off the original axios error - #308

Open
mfal wants to merge 1 commit into
masterfrom
claude/xenodochial-wilbur-bcd7b7
Open

mfal wants to merge 1 commit into
masterfrom
claude/xenodochial-wilbur-bcd7b7

Conversation

@mfal

@mfal mfal commented Sep 8, 2026

Copy link
Copy Markdown
Member

Problem

Request.execute() caught a rejection and did:

const error = AxiosError.from(e);
if (error.isAxiosError && error.response) {
  return error.response as unknown as ResponsePromise<TOp>;
}
throw e;

The comment above it claimed this preserves the previous behavior of returning the response even for HTTP errors. It could not: AxiosError.from(error, code, config, request, response) builds a fresh AxiosError and assigns response only from its own fifth argument, which was not passed here. So error.response was always undefined and the method always rethrew.

Confirmed against the installed axios 1.15.2 (node_modules/axios/lib/core/AxiosError.js):

const orig = Object.assign(new AxiosError("Not Found"), { response: { status: 404 } });
AxiosError.from(orig).response // => undefined

Today this is masked because buildAxiosConfig() sets validateStatus: () => true, so axios never rejects on an HTTP status in the first place — the branch was dead code that would not have protected the intended behavior if that option were ever changed or overridden by a caller's axios config.

Change

Read the response off the original error using axios' named isAxiosError() type guard, keeping the fallback as live code:

if (isAxiosError(e) && e.response) {
  return e.response as unknown as ResponsePromise<TOp>;
}

The type guard also narrows e, so no cast on the error is needed. The comment is reworded to describe the actual arrangement: validateStatus: () => true is the primary mechanism, and this branch is the fallback for callers that override it.

Passing e.response back into AxiosError.from would just rebuild an error object we immediately discard; dropping the branch entirely would make the "return all responses" contract depend solely on no caller ever overriding validateStatus.

Tests

Request.test.ts gains an error handling block covering three cases: the response is returned, an axios error without a response is rethrown, and a non-axios error is rethrown. requestFn had to be typed — an untyped jest.fn() from @jest/globals infers never args and rejects mockRejectedValue.

Verified the new test is not vacuous: with the old AxiosError.from code restored, returns an axios error's response instead of throwing fails (1 failed, 19 passed); with the fix all 20 pass.

yarn nx run @mittwald/api-client-commons:test --skip-nx-cache   # 3 suites, 20 tests, green
yarn nx run @mittwald/api-client-commons:lint --skip-nx-cache   # clean

🤖 Generated with Claude Code

`execute()` used `AxiosError.from(e)` before checking `error.response`.
`AxiosError.from(error, code, config, request, response)` builds a fresh
`AxiosError` and assigns `response` only from its own fifth argument, which
was not passed - so `error.response` was always `undefined` and the branch
never returned a response.

The fallback is currently masked by `validateStatus: () => true`, which keeps
axios from rejecting on an HTTP status at all, so the branch was dead code.
It would not have preserved the intended behavior had a caller overridden
`validateStatus` in its own axios config.

Read the response off the original error via axios' `isAxiosError()` type
guard instead, and reword the comment to describe the actual arrangement.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mfal
mfal marked this pull request as ready for review September 24, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant