Skip to content

refactor(cli): centralize HTTP status error mapping #285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
steipete wants to merge 5 commits into from
Closed

refactor(cli): centralize HTTP status error mapping #285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f500c73
fix(cli): throw Error on timeout aborts
zats Jan 26, 2026
8665d44
fix(http): wrap fetch calls in try-finally to prevent timer leaks
zats Jan 26, 2026
d4bc86d
Merge remote-tracking branch 'origin/main' into fix/cli-timeout
steipete Feb 14, 2026
5476e8a
fix(cli): unify timeout abort handling
steipete Feb 14, 2026
f697310
refactor(cli): centralize HTTP status errors and timeout tests
steipete Feb 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion e2e/clawdhub.e2e.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ async function makeTempConfig(registry: string, token: string | null) {

async function fetchWithTimeout(input: RequestInfo | URL, init?: RequestInit) {
const controller = new AbortController()
const timeout = setTimeout(() => controller.abort('Timeout'), REQUEST_TIMEOUT_MS)
const timeout = setTimeout(() => controller.abort(new Error('Timeout')), REQUEST_TIMEOUT_MS)
try {
return await fetch(input, { ...init, signal: controller.signal })
} finally {
Expand Down
74 changes: 73 additions & 1 deletion packages/clawdhub/src/http.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,41 @@
/* @vitest-environment node */

import { describe, expect, it, vi } from 'vitest'
import { apiRequest, apiRequestForm, downloadZip } from './http'
import { apiRequest, apiRequestForm, downloadZip, fetchText } from './http'
import { ApiV1WhoamiResponseSchema } from './schema/index.js'

function mockImmediateTimeouts() {
const setTimeoutMock = vi.fn((callback: () => void) => {
callback()
return 1 as unknown as ReturnType<typeof setTimeout>
})
const clearTimeoutMock = vi.fn()
vi.stubGlobal('setTimeout', setTimeoutMock as typeof setTimeout)
vi.stubGlobal('clearTimeout', clearTimeoutMock as typeof clearTimeout)
return { setTimeoutMock, clearTimeoutMock }
}

function createAbortingFetchMock() {
return vi.fn(async (_url: string, init?: RequestInit) => {
const signal = init?.signal
if (!signal || !(signal instanceof AbortSignal)) {
throw new Error('Missing abort signal')
}
if (signal.aborted) {
throw signal.reason
}
return await new Promise<Response>((_resolve, reject) => {
signal.addEventListener(
'abort',
() => {
reject(signal.reason)
},
{ once: true },
)
})
})
}

describe('apiRequest', () => {
it('adds bearer token and parses json', async () => {
const fetchMock = vi.fn().mockResolvedValue({
Expand Down Expand Up @@ -92,6 +124,25 @@ describe('apiRequest', () => {
expect(fetchMock).toHaveBeenCalledTimes(1)
vi.unstubAllGlobals()
})

it('aborts with Error timeouts and retries', async () => {
const { clearTimeoutMock } = mockImmediateTimeouts()
const fetchMock = createAbortingFetchMock()
vi.stubGlobal('fetch', fetchMock)

let caught: unknown
try {
await apiRequest('https://example.com', { method: 'GET', path: '/x' })
} catch (error) {
caught = error
}

expect(caught).toBeInstanceOf(Error)
expect((caught as Error).message).toBe('Timeout')
expect(fetchMock).toHaveBeenCalledTimes(3)
expect(clearTimeoutMock.mock.calls.length).toBeGreaterThanOrEqual(3)
vi.unstubAllGlobals()
})
})

describe('apiRequestForm', () => {
Expand Down Expand Up @@ -154,3 +205,24 @@ describe('apiRequestForm', () => {
vi.unstubAllGlobals()
})
})

describe('fetchText', () => {
it('aborts with Error timeouts and retries', async () => {
const { clearTimeoutMock } = mockImmediateTimeouts()
const fetchMock = createAbortingFetchMock()
vi.stubGlobal('fetch', fetchMock)

let caught: unknown
try {
await fetchText('https://example.com', { path: '/x' })
} catch (error) {
caught = error
}

expect(caught).toBeInstanceOf(Error)
expect((caught as Error).message).toBe('Timeout')
expect(fetchMock).toHaveBeenCalledTimes(3)
expect(clearTimeoutMock.mock.calls.length).toBeGreaterThanOrEqual(3)
vi.unstubAllGlobals()
})
})
86 changes: 33 additions & 53 deletions packages/clawdhub/src/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,22 +52,13 @@ export async function apiRequest<T>(
headers['Content-Type'] = 'application/json'
body = JSON.stringify(args.body ?? {})
}
const controller = new AbortController()
const timeout = setTimeout(() => controller.abort('Timeout'), REQUEST_TIMEOUT_MS)
const response = await fetch(url, {
const response = await fetchWithTimeout(url, {
method: args.method,
headers,
body,
signal: controller.signal,
})
clearTimeout(timeout)
if (!response.ok) {
const text = await response.text().catch(() => '')
const message = text || `HTTP ${response.status}`
if (response.status === 429 || response.status >= 500) {
throw new Error(message)
}
throw new AbortError(message)
throwHttpStatusError(response.status, await readResponseTextSafe(response))
}
return (await response.json()) as unknown
},
Expand Down Expand Up @@ -101,22 +92,13 @@ export async function apiRequestForm<T>(

const headers: Record<string, string> = { Accept: 'application/json' }
if (args.token) headers.Authorization = `Bearer ${args.token}`
const controller = new AbortController()
const timeout = setTimeout(() => controller.abort('Timeout'), REQUEST_TIMEOUT_MS)
const response = await fetch(url, {
const response = await fetchWithTimeout(url, {
method: args.method,
headers,
body: args.form,
signal: controller.signal,
})
clearTimeout(timeout)
if (!response.ok) {
const text = await response.text().catch(() => '')
const message = text || `HTTP ${response.status}`
if (response.status === 429 || response.status >= 500) {
throw new Error(message)
}
throw new AbortError(message)
throwHttpStatusError(response.status, await readResponseTextSafe(response))
}
return (await response.json()) as unknown
},
Expand All @@ -138,17 +120,10 @@ export async function fetchText(registry: string, args: TextRequestArgs): Promis

const headers: Record<string, string> = { Accept: 'text/plain' }
if (args.token) headers.Authorization = `Bearer ${args.token}`
const controller = new AbortController()
const timeout = setTimeout(() => controller.abort('Timeout'), REQUEST_TIMEOUT_MS)
const response = await fetch(url, { method: 'GET', headers, signal: controller.signal })
clearTimeout(timeout)
const response = await fetchWithTimeout(url, { method: 'GET', headers })
const text = await response.text()
if (!response.ok) {
const message = text || `HTTP ${response.status}`
if (response.status === 429 || response.status >= 500) {
throw new Error(message)
}
throw new AbortError(message)
throwHttpStatusError(response.status, text)
}
return text
},
Expand All @@ -166,23 +141,38 @@ export async function downloadZip(registry: string, args: { slug: string; versio
return await fetchBinaryViaCurl(url.toString())
}

const controller = new AbortController()
const timeout = setTimeout(() => controller.abort('Timeout'), REQUEST_TIMEOUT_MS)
const response = await fetch(url.toString(), { method: 'GET', signal: controller.signal })
clearTimeout(timeout)
const response = await fetchWithTimeout(url.toString(), { method: 'GET' })
if (!response.ok) {
const message = (await response.text().catch(() => '')) || `HTTP ${response.status}`
if (response.status === 429 || response.status >= 500) {
throw new Error(message)
}
throw new AbortError(message)
throwHttpStatusError(response.status, await readResponseTextSafe(response))
}
return new Uint8Array(await response.arrayBuffer())
},
{ retries: 2 },
)
}

async function fetchWithTimeout(url: string, init: RequestInit): Promise<Response> {
const controller = new AbortController()
const timeout = setTimeout(() => controller.abort(new Error('Timeout')), REQUEST_TIMEOUT_MS)
try {
return await fetch(url, { ...init, signal: controller.signal })
} finally {
clearTimeout(timeout)
}
}

async function readResponseTextSafe(response: Response): Promise<string> {
return await response.text().catch(() => '')
}

function throwHttpStatusError(status: number, text: string): never {
const message = text || `HTTP ${status}`
if (status === 429 || status >= 500) {
throw new Error(message)
}
throw new AbortError(message)
}

async function fetchJsonViaCurl(url: string, args: RequestArgs) {
const headers = ['-H', 'Accept: application/json']
if (args.token) {
Expand Down Expand Up @@ -217,10 +207,7 @@ async function fetchJsonViaCurl(url: string, args: RequestArgs) {
const status = Number(output.slice(splitAt + 1).trim())
if (!Number.isFinite(status)) throw new Error('curl response missing status')
if (status < 200 || status >= 300) {
if (status === 429 || status >= 500) {
throw new Error(body || `HTTP ${status}`)
}
throw new AbortError(body || `HTTP ${status}`)
throwHttpStatusError(status, body)
}
return JSON.parse(body || 'null') as unknown
}
Expand Down Expand Up @@ -272,10 +259,7 @@ async function fetchJsonFormViaCurl(url: string, args: FormRequestArgs) {
const status = Number(output.slice(splitAt + 1).trim())
if (!Number.isFinite(status)) throw new Error('curl response missing status')
if (status < 200 || status >= 300) {
if (status === 429 || status >= 500) {
throw new Error(body || `HTTP ${status}`)
}
throw new AbortError(body || `HTTP ${status}`)
Copy link

@cursor cursor bot Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetchTextViaCurl not refactored to use centralized helper

Low Severity

fetchTextViaCurl still contains the old inline error-handling pattern (if (status === 429 || status >= 500) … throw new AbortError(…)) instead of calling the new throwHttpStatusError helper. Every other curl function (fetchJsonViaCurl, fetchJsonFormViaCurl, fetchBinaryViaCurl) was updated to use the centralized helper, making this the only remaining duplication — which defeats the stated purpose of the refactor and risks divergent bug fixes in the future.

Additional Locations (1)

Fix in Cursor Fix in Web

throwHttpStatusError(status, body)
}
return JSON.parse(body || 'null') as unknown
} finally {
Expand Down Expand Up @@ -344,11 +328,7 @@ async function fetchBinaryViaCurl(url: string) {
if (!Number.isFinite(status)) throw new Error('curl response missing status')
if (status < 200 || status >= 300) {
const body = await readFileSafe(filePath)
const message = body ? new TextDecoder().decode(body) : `HTTP ${status}`
if (status === 429 || status >= 500) {
throw new Error(message)
}
throw new AbortError(message)
throwHttpStatusError(status, body ? new TextDecoder().decode(body) : '')
}
const bytes = await readFileSafe(filePath)
return bytes ? new Uint8Array(bytes) : new Uint8Array()
Expand Down