feat(cli): honor HTTPS_PROXY/HTTP_PROXY/NO_PROXY behind corporate and CI proxies#169
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR adds proxy environment handling at startup by introducing an undici-based dispatcher installer, wiring it into the CLI entrypoint, adding the runtime dependency, and expanding tests for proxy detection, NO_PROXY coexistence, and failure handling. ChangesProxy Agent Support
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/proxy.ts (1)
16-17: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAvoid
EnvHttpProxyAgenton the CLI path for now.undici@8.5.0still emits the[UNDICI-EHPA]experimental warning on proxy-enabled starts, which adds avoidable stderr noise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/proxy.ts` around lines 16 - 17, Avoid using EnvHttpProxyAgent in the CLI proxy setup because undici@8.5.0 still emits the [UNDICI-EHPA] warning on proxy-enabled starts. Update src/lib/proxy.ts so the proxy initialization path used by the CLI relies on setGlobalDispatcher without importing or constructing EnvHttpProxyAgent, and keep the dispatcher logic in the proxy-related helper(s) isolated so the warning is not triggered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/proxy.test.ts`:
- Around line 1-34: The current proxy tests only cover proxy-variable detection
in maybeInstallProxyAgent and miss the required NO_PROXY coverage. Add a test
that exercises actual dispatcher installation and NO_PROXY bypass behavior by
constructing an EnvHttpProxyAgent (or invoking deps.install through
maybeInstallProxyAgent) with NO_PROXY set, then assert a matching host is routed
around the proxy while the proxy-install path still behaves as expected.
In `@src/lib/proxy.ts`:
- Around line 30-41: The proxy setup in maybeInstallProxyAgent should not let
malformed or unsupported proxy environment values abort startup; wrap the
EnvHttpProxyAgent installation in a try/catch and fall back to the default
dispatcher when it fails. Keep the existing proxy detection logic in
maybeInstallProxyAgent, but ensure the call to install(new EnvHttpProxyAgent())
is protected so bad HTTP_PROXY/HTTPS_PROXY values do not propagate out through
src/index.ts and stop CLI execution.
---
Nitpick comments:
In `@src/lib/proxy.ts`:
- Around line 16-17: Avoid using EnvHttpProxyAgent in the CLI proxy setup
because undici@8.5.0 still emits the [UNDICI-EHPA] warning on proxy-enabled
starts. Update src/lib/proxy.ts so the proxy initialization path used by the CLI
relies on setGlobalDispatcher without importing or constructing
EnvHttpProxyAgent, and keep the dispatcher logic in the proxy-related helper(s)
isolated so the warning is not triggered.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 54693c93-d286-4eb7-b932-7f090a38c504
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/index.tssrc/lib/proxy.test.tssrc/lib/proxy.ts
| import { describe, expect, it, vi } from 'vitest'; | ||
| import { maybeInstallProxyAgent } from './proxy.js'; | ||
|
|
||
| describe('maybeInstallProxyAgent', () => { | ||
| it('installs an agent when HTTPS_PROXY is set', () => { | ||
| const install = vi.fn(); | ||
| const installed = maybeInstallProxyAgent({ | ||
| env: { HTTPS_PROXY: 'http://proxy.corp.example.com:8080' }, | ||
| install, | ||
| }); | ||
| expect(installed).toBe(true); | ||
| expect(install).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it.each(['https_proxy', 'HTTP_PROXY', 'http_proxy'] as const)( | ||
| 'also honors the %s spelling', | ||
| name => { | ||
| const install = vi.fn(); | ||
| const installed = maybeInstallProxyAgent({ | ||
| env: { [name]: 'http://proxy.corp.example.com:8080' }, | ||
| install, | ||
| }); | ||
| expect(installed).toBe(true); | ||
| expect(install).toHaveBeenCalledTimes(1); | ||
| }, | ||
| ); | ||
|
|
||
| it('does nothing when no proxy variable is set (default path unchanged)', () => { | ||
| const install = vi.fn(); | ||
| expect(maybeInstallProxyAgent({ env: {}, install })).toBe(false); | ||
| expect(maybeInstallProxyAgent({ env: { HTTPS_PROXY: '' }, install })).toBe(false); | ||
| expect(install).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing NO_PROXY test coverage per stated objective.
The linked issue's acceptance criteria call for tests verifying "dispatcher installation and NO_PROXY behavior," but this suite only covers proxy-var detection/no-op cases — no test exercises NO_PROXY exemptions. Since that logic lives inside EnvHttpProxyAgent itself, a meaningful test would need to construct a real agent with NO_PROXY set and assert it bypasses the proxy for a matching host, rather than only checking maybeInstallProxyAgent's boolean return.
Want me to draft a test that constructs EnvHttpProxyAgent directly (or via deps.install) and asserts requests to a NO_PROXY-listed host bypass the proxy dispatch?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/proxy.test.ts` around lines 1 - 34, The current proxy tests only
cover proxy-variable detection in maybeInstallProxyAgent and miss the required
NO_PROXY coverage. Add a test that exercises actual dispatcher installation and
NO_PROXY bypass behavior by constructing an EnvHttpProxyAgent (or invoking
deps.install through maybeInstallProxyAgent) with NO_PROXY set, then assert a
matching host is routed around the proxy while the proxy-install path still
behaves as expected.
… instead of crashing startup
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Direction approved — an explicit One blocker: |
…ires Node >=22.19)
|
Thanks for running CI — fixed the Node 20 failure. The |
2 participants
What does this PR do?
Node's built-in fetch (undici) ignores the proxy environment variables, so
behind a corporate or CI proxy every TestSprite request dies with
fetch failedafter a full retry cycle. This installs undici's
EnvHttpProxyAgentas theglobal dispatcher at startup only when a proxy variable is present, honoring
HTTPS_PROXY/HTTP_PROXY(upper and lower case) andNO_PROXYexemptions. Noproxy var set → no-op, default path byte-identical, zero startup cost.
Related issue
Fixes #119
Type of change
Checklist
mainbranch.npm run lintandnpm run format:checkpass.npm run typecheckpasses.npm testpasses and coverage stays at or above the 80% gate.Notes for reviewers
Adds
undicias an explicit runtime dependency (the same engine Node alreadybundles for fetch) — per the triage note on #119 approving this. The installer
is dependency-injected (
ProxyDeps.install) so the unit tests never touch theglobal dispatcher.
EnvHttpProxyAgentreadsNO_PROXYitself per request, soexemptions work with no extra plumbing. I'm the assignee of #119.
Summary by CodeRabbit
HTTP_PROXY/HTTPS_PROXY(including uppercase and lowercase variants).NO_PROXYbehavior, and installer failure/warning behavior.undiciruntime dependency to support proxy-aware fetching.