Proxy tests assert a code path production never executes (decideProxy vs EnvHttpProxyAgent)
- Dominant language
- JavaScript
- Stars
- 29.3k
- Forks
- 2.9k
- Avg merge
- 15h 36m
- Merged PRs (30d)
- 70
Description
Found while auditing `tsc --noUnused*` hits for the Occam/simplification sweep (task #307). Filing separately because fixing it is a **behavioural decision**, not dead-code removal, so it should not ride along with the P1b cleanup PR.
## The finding
`src/node-network.ts` contains **two parallel proxy-decision implementations**, and the test suite exercises the one production never reaches.
**Production path** (`fetchWithNodeNetwork`):
```
:202 dispatcher: getDispatcherForUrl(url)
→:188 getDispatcherForUrl() // ignores its url arg entirely
→:156 new EnvHttpProxyAgent({ httpProxy, httpsProxy, noProxy })
```
Protocol selection and NO_PROXY matching are performed **inside undici's agent**, per request, at dispatch time.
**Tested path** (`decideProxy(url, env)`): URL-aware, implements NO_PROXY matching, loopback bypass, protocol selection and ALL_PROXY fallback by hand.
`getDispatcherForUrl` **never calls `decideProxy`**.
## Evidence
```
$ grep -rn 'decideProxy' src/ | wc -l # 12
$ grep -rn 'decideProxy' src/ | grep -c test # 11
$ grep -rn 'decideProxy' src/ | grep -v test
src/node-network.ts:177:export function decideProxy(...) # the definition itself
```
So: **zero production callers.** `src/node-network.ts` is also absent from `package.json#exports`, so it is not public API either.
The assertions currently riding on it include:
- `routes external https traffic through https_proxy`
- `falls back to HTTP_PROXY for https traffic when HTTPS_PROXY is absent`
- `bypasses proxies for loopback addresses`
- `honors NO_PROXY domain matches`
- `supports wildcard-style NO_PROXY subdomain entries`
- `matches NO_PROXY entries that rely on the default URL port`
- `falls back to ALL_PROXY when protocol-specific settings are absent`
## Why it matters
This is not merely redundant code. It produces **false assurance in a hard-to-debug area**: if undici's `EnvHttpProxyAgent` ever changes its NO_PROXY or protocol-selection semantics — or if our env parsing and undici's disagree about an edge case — every one of the assertions above still passes while real traffic routes differently. "NO_PROXY is covered by tests" is currently overstated.
It is the same failure shape catalogued elsewhere in this sweep (see #2319): **the thing asserted and the thing that can actually break are not the same thing.**
## Options (needs a decision, hence a separate issue)
1. **Make production reuse `decideProxy`** — consult it before/instead of delegating to the agent, so the existing tests genuinely cover the shipped path. Highest fidelity; changes runtime behaviour and needs care.
2. **Re-point the tests at real behaviour** — assert what `EnvHttpProxyAgent` actually does for representative URLs, then delete `decideProxy` as redundant. Keeps runtime untouched; makes coverage honest.
3. Keep both, and document `decideProxy` explicitly as a spec/reference implementation that is **not** the production path — weakest, since nothing prevents silent divergence.
Option 2 looks most aligned with the Occam goal (one implementation, tests pinned to it), but this is a behavioural call rather than a mechanical cleanup.
## Explicitly out of scope
Do **not** fold this into the P1b dead-code PR. That PR's value rests on being provably behaviour-neutral; touching proxy semantics inside it would destroy that property. The unused `url` parameter on `getDispatcherForUrl` is safe to drop there on its own — the dispatcher cache key is `[httpProxy, httpsProxy, noProxy]` with no URL component, so removing the argument changes no caching or selection behaviour.
Contributor guide
Research direction
Start in src/node-network.ts at fetchWithNodeNetwork, getDispatcherForUrl, EnvHttpProxyAgent, and decideProxy, then inspect the listed tests that call decideProxy. Compare the tested behavior with the production dispatch path and determine which approach the project should retain. Done means the tests exercise shipped proxy behavior and the unused or redundant path is resolved.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- node.js, typescript
- Domain
- cli, networking, testing-qa
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100