Conversation
Use a cached proxy-aware dispatcher for CLI-owned fetch requests in Node so built-in traffic respects HTTP_PROXY, HTTPS_PROXY, and NO_PROXY without global dispatcher changes. This routes API and OAuth requests through shared transport helpers and keeps timeout and dispatcher behavior in one place.
There was a problem hiding this comment.
Pull request overview
Adds proxy environment variable support (HTTP_PROXY, HTTPS_PROXY, NO_PROXY) to the HTTP client by introducing an undici-based dispatcher layer and a fetchWithRetry wrapper, then migrating existing fetch calls to use it.
Changes:
- New
http-dispatcher.tsthat selects betweenAgentandEnvHttpProxyAgentbased on proxy env vars - New
fetchWithRetry.tswrapper with timeout and retry support, injecting the dispatcher into Node.jsfetch - Migrated
api.tsandoauth.tsfrom rawfetchtofetchWithRetry
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/transport/http-dispatcher.ts | New module selecting undici dispatcher based on proxy env vars |
| src/transport/fetch-with-retry.ts | New fetch wrapper with retry, timeout, and dispatcher injection |
| src/lib/oauth.ts | Switched from fetch to fetchWithRetry |
| src/lib/api.ts | Switched from fetch to fetchWithRetry |
| src/tests/http-dispatcher.test.ts | Tests for dispatcher selection and caching |
| src/tests/fetch-with-retry.test.ts | Tests for retry, timeout, and dispatcher usage |
| src/tests/oauth.test.ts | Tests for oauth using fetchWithRetry |
| src/tests/api.test.ts | Updated tests to mock fetchWithRetry |
| package.json, package-lock.json, pnpm-lock.yaml | Added undici 7.22.0 dependency |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![doist-bot[bot]](https://avatars.githubusercontent.com/in/2345791?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR brings a welcome update by making the built-in HTTP client honor proxy environment variables by default, which greatly improves network configurability. The implementation is well-structured, though a few adjustments would further enhance its stability and compatibility. There are some notes to review regarding Node 18 compatibility with the undici upgrade, properly identifying timeout errors for retries, cleaning up signal event listeners to prevent memory leaks, and ensuring timeouts are cleared immediately when errors are caught.
| clear: () => void; | ||
| } { | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => { |
There was a problem hiding this comment.
[P2] Enable retrying on timeout errors
Currently, when a timeout occurs, this throws a generic Error. The isNetworkError function (which determines if an error should be retried) only checks for TypeError. Consequently, if retries > 0 is ever configured, timeout errors will not be retried because they don't pass the check.
You can fix this by explicitly naming the timeout error (as suggested below) and updating isNetworkError to also accept error.name === "TimeoutError". You will also need to update the expect(...).rejects.toThrow(...) in fetch-with-retry.test.ts to match the new error.
| const timeoutId = setTimeout(() => { | |
| const timeoutId = setTimeout(() => { | |
| const error = new Error(`Request timeout after ${timeoutMs}ms`); | |
| error.name = "TimeoutError"; | |
| controller.abort(error); | |
| }, timeoutMs); |
| controller.abort(new Error(`Request timeout after ${timeoutMs}ms`)); | ||
| }, timeoutMs); | ||
|
|
||
| function clear(): void { |
There was a problem hiding this comment.
[P2] Memory leak in existingSignal event listener
When existingSignal is provided, an abort event listener is attached to it. However, if the request completes successfully or fails before the signal aborts, clear() is called to remove the timeout but it does not remove the event listener from existingSignal. If a long-lived signal is passed to multiple requests, this will leak closures and eventually cause memory issues.
Store a reference to the handler and explicitly remove it during cleanup.
| function clear(): void { | |
| let abortHandler: (() => void) | undefined; | |
| function clear(): void { | |
| clearTimeout(timeoutId); | |
| if (existingSignal && abortHandler) { | |
| existingSignal.removeEventListener("abort", abortHandler); | |
| } | |
| } | |
| if (existingSignal) { | |
| if (existingSignal.aborted) { | |
| clearTimeout(timeoutId); | |
| controller.abort(existingSignal.reason); | |
| } else { | |
| abortHandler = () => { | |
| clearTimeout(timeoutId); | |
| controller.abort(existingSignal.reason); | |
| }; | |
| existingSignal.addEventListener("abort", abortHandler, { once: true }); | |
| } | |
| } |
| } | ||
|
|
||
| return response; | ||
| } catch (error) { |
There was a problem hiding this comment.
[P2] Clear timeout immediately upon catching error
The clearTimeoutFn is called at multiple exit points in the catch block. Notably, if the request is going to be retried, the timeout is cleared after the retryDelay wait. If the delay is long, the timeout for the failed request could fire during the wait, executing an unnecessary abort() on the discarded controller.
It is safer and cleaner to clear the timeout immediately upon catching the error, removing the need for redundant checks later in the block.
| } catch (error) { | |
| } catch (error) { | |
| if (clearTimeoutFn) { | |
| clearTimeoutFn(); | |
| } | |
| lastError = error as Error; | |
| const shouldRetry = | |
| attempt < config.retries && config.retryCondition(lastError); | |
| if (!shouldRetry) { | |
| throw lastError; | |
| } | |
| const delay = config.retryDelay(attempt + 1); | |
| if (delay > 0) { | |
| await wait(delay); | |
| } | |
| } |
| "marked": "15.0.12", | ||
| "marked-terminal": "7.3.0", | ||
| "open": "10.2.0", | ||
| "undici": "7.22.0", |
There was a problem hiding this comment.
[P1] undici@7 makes the CLI unusable on Node 18. The previous commit still starts on v18.20.4, but this branch now aborts on ol --help with ReferenceError: File is not defined from undici during module load because the command modules eagerly import the new transport code. Please either pin to an undici major that still supports Node 18 or explicitly raise/document the package's minimum Node version.
|
I'm not familiar with these env vars. And is this something we'd need to do for all our CLIs? |
Makes the HTTP client honor
HTTP_PROXY,HTTPS_PROXY, andNO_PROXYby default for its built-in network requests.