From 5456400dad6337904d8365fdc3c335d97a3d2f86 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 05:33:15 +0000 Subject: [PATCH] fix(proxy): the proxy notice follows Node's measured rules; tests stop reading the host's proxy env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured with a CONNECT-logging proxy on the official 20.20.2, 21.7.3, 22.20.0, 22.21.0, 22.22.2, 23.11.1, 24.0.0, 24.4.1 and 24.5.0 binaries. The notice (PHARN-12) was wrong in several cases; now: - Which variable: undici's own `??` lookup over the exact spellings — https_proxy ?? HTTPS_PROXY, then http_proxy ?? HTTP_PROXY (an https fetch falls back to the http pair; an empty https_proxy shadows HTTPS_PROXY). Lowercase wins. A mixed-case name like Https_Proxy is reported as ignored, never as proxied. HTTP_PROXY alone is reported. - Whether it is used: where Node lists --use-env-proxy (22.21+, 24.5+) the LAST --use-env-proxy / --no-use-env-proxy token decides (`-` or `_`, NODE_OPTIONS then the command line), else NODE_USE_ENV_PROXY === '1'. On 24.0-24.4 (no flag yet) any non-empty NODE_USE_ENV_PROXY turns it on. An opt-out names --no-use-env-proxy instead of the variable already set. - tests/setup/hermetic-env.ts (vitest setupFiles) deletes the proxy variables and NODE_OPTIONS before every test file: under NODE_USE_ENV_PROXY=1 the suite had 15 failures, under an exported HTTPS_PROXY 1; now 0. LIMITS.md §3a (human-only) still says a proxy variable changes nothing; flagged for the maintainer in the PR. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0199owRmYfskqYQVQrVP679o --- .dev/features/proxy-notice-truth/GRILL.md | 63 ++++ .dev/features/proxy-notice-truth/PLAN.md | 138 +++++++++ .../features/proxy-notice-truth/REGRESSION.md | 44 +++ .dev/features/proxy-notice-truth/REVIEW.md | 63 ++++ .dev/features/proxy-notice-truth/SHIP.md | 24 ++ .dev/features/proxy-notice-truth/VERIFY.md | 49 ++++ .../proxy-notice-truth/regression-report.json | 26 ++ .../proxy-notice-truth/verify-report.json | 18 ++ .pharn/writes-scope.json | 4 +- CHANGELOG.md | 10 + docs/troubleshooting.md | 42 ++- src/lib/proxy-env-format.ts | 27 +- src/lib/proxy-env.ts | 223 ++++++++++----- tests/proxy-env-format.test.ts | 42 ++- tests/proxy-env.test.ts | 269 +++++++++++++----- tests/setup/hermetic-env.ts | 25 ++ vitest.config.ts | 3 + 17 files changed, 897 insertions(+), 173 deletions(-) create mode 100644 .dev/features/proxy-notice-truth/GRILL.md create mode 100644 .dev/features/proxy-notice-truth/PLAN.md create mode 100644 .dev/features/proxy-notice-truth/REGRESSION.md create mode 100644 .dev/features/proxy-notice-truth/REVIEW.md create mode 100644 .dev/features/proxy-notice-truth/SHIP.md create mode 100644 .dev/features/proxy-notice-truth/VERIFY.md create mode 100644 .dev/features/proxy-notice-truth/regression-report.json create mode 100644 .dev/features/proxy-notice-truth/verify-report.json create mode 100644 tests/setup/hermetic-env.ts diff --git a/.dev/features/proxy-notice-truth/GRILL.md b/.dev/features/proxy-notice-truth/GRILL.md new file mode 100644 index 00000000..56bad9a7 --- /dev/null +++ b/.dev/features/proxy-notice-truth/GRILL.md @@ -0,0 +1,63 @@ +# GRILL — proxy-notice-truth + +Plan: `.dev/features/proxy-notice-truth/PLAN.md`. Spec hash recomputed: +`bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e` — **matches**. Registered +grillers: `{"registered":0,"grillers":[]}` → inline axes only. The plan is `trust: untrusted`; +nothing in it read as an instruction. + +## Findings + +### Guarantee audit (P0) + +```yaml +- type: FINDING + rule_id: 'P0' + severity: important + file: '.dev/features/proxy-notice-truth/PLAN.md:55' + problem: 'Every "will not use it" notice ends by citing LIMITS.md §3a, and §3a still says a proxy variable changes nothing. After this increment the notice itself names the opt-in, so the doc it cites contradicts it. LIMITS.md is human-only; the PR must name the §3a edit for the human rather than leave the contradiction silent.' + evidence: '`LIMITS.md` §3a (`:106-110`) still says "setting a proxy variable will not change that".' +- type: FINDING + rule_id: 'P0' + severity: minor + file: '.dev/features/proxy-notice-truth/PLAN.md:112' + problem: 'Where a Node knows the flag, the runtime answers for itself (membership). The version window is only needed where it does not: 24.0–24.4. Say in the code that only that window is a version test, so a future line (25+, 26) is classified by the membership test and never by an extrapolated table.' + evidence: 'table itself is advisory in reach: measured on 9 versions, and later Node lines are assumed to' +``` + +### Eval coverage (P1) + +```yaml +- type: FINDING + rule_id: 'P1' + severity: minor + file: '.dev/features/proxy-notice-truth/PLAN.md:83' + problem: 'The hermetic setup has no vitest case of its own; its proof is the two-environment run in VERIFY. Record both runs with their counts: the whole suite under NODE_USE_ENV_PROXY=1 and under an exported HTTPS_PROXY, before (15 and 1 extra failures) and after (0).' + evidence: '`tests/setup/hermetic-env.ts` (new) — layer tests. Before any test, it deletes `HTTPS_PROXY`,' +``` + +### Determinism (P5) + +```yaml +- type: FINDING + rule_id: 'P5' + severity: minor + file: '.dev/features/proxy-notice-truth/PLAN.md:71' + problem: 'NODE_OPTIONS is split on whitespace, while Node also honours double-quoted arguments. A flag token can never contain a space, so this cannot mis-read a real flag, but a quoted VALUE that happens to spell the flag would be read as one. Pathological; state the approximation in the comment.' + evidence: '(regex `^--(no[-_])?use[-_]env[-_]proxy(=.*)?$` over `NODE_OPTIONS` tokens, then the command' +``` + +### Checked, no finding + +- Trust (P2): a printed variable NAME reaches output only when it case-folds to one of four proxy + names (ASCII letters and `_`). Values still go through `redactProxyUrl`. +- Axis (P3): the detection logic changes only in `proxy-env.ts`, and the wording only in + `proxy-env-format.ts` — the file split's own rule. + +## Summary + +The rule set is measured, and the plan keeps pure functions pure. The important gap is outside the +plan's reach: `LIMITS.md` §3a, which every notice cites, will contradict the notice. That has to be +handed to the human explicitly. The rest are comments and evidence to record. + +**ADVISORY VERDICT: 4 concerns raised (0 blocking-severity, 1 important, 3 minor) — for the human to +weigh before /pharn-dev-build.** diff --git a/.dev/features/proxy-notice-truth/PLAN.md b/.dev/features/proxy-notice-truth/PLAN.md new file mode 100644 index 00000000..273ccb15 --- /dev/null +++ b/.dev/features/proxy-notice-truth/PLAN.md @@ -0,0 +1,138 @@ +# PLAN — proxy-notice-truth (the proxy notice follows Node's measured rules; its tests stop reading the host's env) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: `detectProxyNotice` reports what Node's `fetch` will actually do. It follows the + measured rule set below: the variable Node reads and its precedence, both flag spellings, the + `--no-` negation and last-one-wins, the 24.0–24.4 window where the variable works without the + flag, and exact-case lookup. The command tests stop depending on the host's proxy environment. +- layer(s): the CLI itself (`src/lib/proxy-env.ts`, `src/lib/proxy-env-format.ts`), tests, docs +- constitution_refs: [P0, P1, P5, P6] + +## Discovery — measured this run (P6) + +The measurement harness is in scratchpad `plan-E/`. A local proxy logs `CONNECT`s, and a client +`fetch`es `https://example.com` under `env -i`. It ran on the official binaries for 20.20.2, 21.7.3, +22.20.0, 22.21.0, 22.22.2, 23.11.1, 24.0.0, 24.4.1 and 24.5.0. + +| Node | flag known¹ | `NODE_USE_ENV_PROXY` turns it on | flag turns it on² | +| ---------------------------- | ----------- | -------------------------------- | ------------------ | +| 20.x, 21.x, 22.0–22.20, 23.x | no | never | startup error | +| 22.21+ (22.22.2), 24.5+ | yes | only the value `1` | yes, last one wins | +| 24.0.0–24.4.x | no | ANY non-empty value (even `0`) | startup error | + +¹ `process.allowedNodeEnvironmentFlags.has('--use-env-proxy')`. ² `--use-env-proxy` or +`--use_env_proxy`, from `NODE_OPTIONS` or the command line. + +The rows below hold wherever env proxy is on (measured on 22.22.2, 24.0.0, 24.4.1 and 24.5.0): + +- **Negation.** `--no-use-env-proxy` or `--no-use_env_proxy` overrides `NODE_USE_ENV_PROXY=1`. +- **Order.** The command line is read after `NODE_OPTIONS`, so the last token wins. + `--use-env-proxy=false` still turns it ON. +- **Case.** `Https_Proxy` is never read on Linux, so the connection goes DIRECT. +- **Precedence.** `https_proxy` beats `HTTPS_PROXY`. Seen via different ports: lowercase won. +- **Fallback.** With only `HTTP_PROXY` set, https requests still go through that proxy (undici's + `EnvHttpProxyAgent` falls back from the https agent to the http one). +- **Exclusion.** `NO_PROXY=example.com` → direct. + +Checked against HEAD `abb274a` (`proxy-env.ts:67-94`): + +- `supportsEnvProxy` is the flag membership alone. So 24.0–24.4 read as `unsupported` ("this Node + has no NODE_USE_ENV_PROXY support") while the variable really does route `fetch`. +- `hasFlagToken` knows only the hyphen spelling and no negation. +- `proxyVariantKeys` matches any letter-case and prefers `HTTPS_PROXY`. So it names a variable Node + ignores, or the one Node does not use. +- It never looks at `http_proxy`/`HTTP_PROXY`. + +F27, reproduced here (proxy variables unset, root capabilities dropped): + +- `NODE_USE_ENV_PROXY=1` → 15 extra failures (add 2, init 2, status 5, update 6). Every one is a + notice test that expects "will not use it". +- `HTTPS_PROXY` exported → `init.test.ts:871` "prints nothing extra…" fails. The notice counts as an + unexpected `log.warn`. +- The four update failures that appear under both are the known root-only chmod cases (the + CI-equivalent wrapper passes them). + +`LIMITS.md` §3a (`:106-110`) still says "setting a proxy variable will not change that". That has +been untrue since PHARN-12. LIMITS.md is human-only (hook-protected), so this plan **surfaces** it +and does not edit it. + +## Files + +- `src/lib/proxy-env.ts` — layer CLI/lib. Changes: + - The runtime record gains the Node version. + - The proxy Node would use for an https URL is resolved with undici's own `??` chains, by exact + key: the first PRESENT of `https_proxy` / `HTTPS_PROXY`; if that is absent or empty, the first + present of `http_proxy` / `HTTP_PROXY` (GATE 1 answer 1 → a). Measured on 22.22.2: an empty + `https_proxy` shadows a set `HTTPS_PROXY` and falls through to the http pair. On win32, + `process.env` lookups are case-insensitive, so the platform difference comes for free. + - A key that only case-folds to one of those four, when no exact lookup found a proxy, yields a + new "ignored spelling" notice. + - The env-proxy state follows the table. Where the flag is known, the LAST flag token decides + (regex `^--(no[-_])?use[-_]env[-_]proxy(=.*)?$` over `NODE_OPTIONS` tokens, then the command + line); with no token, `NODE_USE_ENV_PROXY === '1'` decides. Where the flag is unknown and the + version is 24.0–24.4, any non-empty `NODE_USE_ENV_PROXY` turns it on. Otherwise: unsupported. +- `src/lib/proxy-env-format.ts` — layer CLI/lib. The message for the ignored spelling ("Node reads + only https_proxy / HTTPS_PROXY / http_proxy / HTTP_PROXY"). The unsupported wording names the + Node lines that have the option (22.21+, 24+), rather than "a newer release". The "on" wording + keeps its NO_PROXY hedge (GATE 1 answer 2 → a). +- `tests/proxy-env.test.ts` — layer tests. One case per measured row; each is an injected runtime, + so the cases are pure. FAIL on base: 24.4 + `NODE_USE_ENV_PROXY=1` → on; `--use_env_proxy` → on; + `NUEP=1` + `--no-use-env-proxy` → available; `Https_Proxy` → ignored spelling; lower beats + upper; `HTTP_PROXY` only → a notice; an empty `https_proxy` shadows `HTTPS_PROXY`. +- `tests/proxy-env-format.test.ts` — layer tests. The new and changed wordings. +- `tests/setup/hermetic-env.ts` (new) — layer tests. Before any test, it deletes `HTTPS_PROXY`, + `https_proxy`, `HTTP_PROXY`, `http_proxy`, `NO_PROXY`, `no_proxy` and `NODE_USE_ENV_PROXY` from + the test process, so `vi.unstubAllEnvs()` restores "absent", never the host's value. +- `vitest.config.ts` — layer tests. Registers that file under `setupFiles`. +- `tests/add.test.ts` — layer tests. Adjusted only if a notice assertion depends on the host Node's + flag support. Today they match "will not use it", which holds for both available and + unsupported. +- `tests/init.test.ts` — layer tests. The same. +- `tests/status.test.ts` — layer tests. The same. +- `tests/update.test.ts` — layer tests. The same. +- `docs/troubleshooting.md` — layer docs. The proxy section: the version table in prose, the + exact-case rule, lowercase precedence, the `HTTP_PROXY` fallback, and `--no-use-env-proxy`. +- `CHANGELOG.md` — `[Unreleased]` → `### Fixed` (notice), plus a line for the hermetic tests + +## Contracts satisfied + +- PHARN-12's own contract: "the notice is one of three TRUE forms" (`proxy-env-format.ts:46-51`). + It is true again on every measured Node, now as four forms (cited, P4). + +## Evals to write (P1) + +- Listed under Files. Six cases FAIL on the base. +- The hermetic setup is proven by running the whole suite under `NODE_USE_ENV_PROXY=1` and under + an exported `HTTPS_PROXY`, before and after. That run is recorded in VERIFY, not a vitest case: + a test cannot set its own process env before its setup file runs. + +## Guarantee audit (P0) + +- "the notice matches Node's behaviour" → floor: pure-function tests over the MEASURED table. The + table itself is advisory in reach: measured on 9 versions, and later Node lines are assumed to + follow the 24.5 row. That assumption is named in the code comment and the docs. +- "the suite's result does not depend on the host's proxy env" → floor: the setup file + (deterministic deletion) plus the two-environment run above. + +## Trust audit (P2) + +- The environment is attacker-influenceable. Values are still printed only through + `redactProxyUrl`. A variable NAME is printed only when it case-folds to a proxy name, so it + contains ASCII letters and `_` only. + +## Determinism audit (P5) + +- Key membership, a regex over tokens, and a version-range compare. No classification. With nothing + set the result is `null` (silence). + +## Open questions (HALT) + +None open. Resolved at GATE 1 (human, 2026-09-25): every question below → **(a)**, the +recommended answer. Kept for the record: + +1. With only `HTTP_PROXY`/`http_proxy` set (no https variable), today pharn says nothing. Node, + when on, still proxies https through it. (a) Report it, naming that variable — recommended, + because it is the proxy Node actually uses. (b) Stay https-only, as today. +2. `NO_PROXY` covering `github.com` hosts while env proxy is on. (a) Keep the current hedge + ("Node's fetch then also honours NO_PROXY") — recommended, no new matching logic. (b) Evaluate + `NO_PROXY` against the three hosts and say "direct" when they match. diff --git a/.dev/features/proxy-notice-truth/REGRESSION.md b/.dev/features/proxy-notice-truth/REGRESSION.md new file mode 100644 index 00000000..1d611597 --- /dev/null +++ b/.dev/features/proxy-notice-truth/REGRESSION.md @@ -0,0 +1,44 @@ +# REGRESSION — proxy-notice-truth + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `f1e8b92cfba1c59c19b1743cde23e77c54ac64bb` (`HEAD` — `origin/main` after #220; the + build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): + - `src/lib/proxy-env.ts`, `src/lib/proxy-env-format.ts` + - `tests/proxy-env.test.ts`, `tests/proxy-env-format.test.ts` + - `tests/setup/hermetic-env.ts` (new), `vitest.config.ts` + - `docs/troubleshooting.md`, `CHANGELOG.md` + + The four command test files the plan also declared needed no change. + +- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook + scratch) and this feature's own stage artifacts are not build output. +- **outside gates:** the 46 stdlib `*.test.mjs` / `*.test.cjs` files `scope` returned (754 tests) + + whole-repo `validate`; 0 committed eval pairs. +- **style-gate skip:** `inside` touches no shared style config (`vitest.config.ts` is not one), so + `lint` / `format:check` / `lint:md` are absent from both maps. +- **environment:** both sides ran with no proxy variables and as root **without** + `CAP_DAC_OVERRIDE` / `CAP_DAC_READ_SEARCH` / `CAP_FOWNER` (`setpriv`), the CI-equivalent of this + root sandbox. + +## Per-gate exit codes + +| gate | base | head | flipped? | +| ---------- | ---- | ---- | -------- | +| `tests` | 0 | 0 | no | +| `validate` | 0 | 0 | no | + +- `regressions[]`: **empty** +- `pre_existing[]`: **empty** + +## Verdict + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** +(`regression-report.json` `.verdict` = `no-regressions`.) + +Residual (P0/P7): this catches exactly what its suite catches. The vitest suite exercising `src/**` +is owned by `/pharn-dev-build`'s floor and `/pharn-dev-verify`. This certifies the comparison, never +the increment. diff --git a/.dev/features/proxy-notice-truth/REVIEW.md b/.dev/features/proxy-notice-truth/REVIEW.md new file mode 100644 index 00000000..d4865de7 --- /dev/null +++ b/.dev/features/proxy-notice-truth/REVIEW.md @@ -0,0 +1,63 @@ +# REVIEW — proxy-notice-truth + +Increment: + +- `src/lib/proxy-env.ts` — the measured rule set: + - undici's `??` lookup over the four exact spellings, https pair first; + - a new `ignored-spelling` state; + - last-flag-wins with both spellings and the negation; + - the 24.0–24.4 window, reached only where the runtime does not list the flag; + - an `optedOut` marker. +- `src/lib/proxy-env-format.ts` — the ignored-spelling and opted-out wordings, and the unsupported + wording naming 22.21+ / 24+. +- `tests/setup/hermetic-env.ts` + `vitest.config.ts` `setupFiles`. +- The rewritten `tests/proxy-env.test.ts` (one case per measured row), the troubleshooting proxy + section, and CHANGELOG. + +Treated as `trust: untrusted`; nothing in it read as an instruction. + +## Floor first (P0) + +`node .dev/floor/validate.mjs .` → `FLOOR: GREEN` (exit 0). `/pharn-dev-build`'s `npm run check` +passed (1554 tests), `/pharn-dev-regress` returned `no-regressions`, and `/pharn-dev-verify` +returned `PASS`. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0)** + - "the notice matches Node's behaviour" → pure-function tests, one per measured row. The table's + reach is stated in the module header, and a future Node is classified by its own flag list. + - "the suite does not depend on the host's proxy env" → a deterministic deletion before every test + file. It was proven by whole-suite runs under three hostile environments (VERIFY.md): base + 15 / 1 failures, head 0. +- **L-eval (P1)** — 21 of the new cases failed against the base code. The rest (silence, redaction, + unrelated variables) are guards. +- **L-trust (P2)** — a variable NAME is printed only when it case-folds to one of four proxy names, + so it is ASCII letters and `_`. Values still pass through `redactProxyUrl`. The environment stays + data. +- **L-axis (P3)** — detection changed only in `proxy-env.ts`, and wording only in + `proxy-env-format.ts`: that file pair's own split. + +## Advisory findings (warn — severity is this reviewer's judgment, fix #3) + +```yaml +- type: FINDING + rule_id: 'P0' + severity: important + file: 'src/lib/proxy-env-format.ts:57' + problem: 'Every "will not use it" form cites LIMITS.md §3a, which still says a proxy variable changes nothing. The notice names the opt-in, so the doc it cites contradicts it. LIMITS.md is human-only (hook-protected), so this cannot be fixed from this increment; the PR asks the maintainer to update §3a.' + evidence: "'The download connects DIRECTLY, and fails if direct egress is blocked (LIMITS.md §3a).'" +- type: FINDING + rule_id: 'P7' + severity: minor + file: 'src/lib/proxy-env.ts:159' + problem: "The win32 behaviour (a mixed-case key is read, because process.env is case-insensitive there) follows from Node's documented process.env semantics but was not measured. CI has no Windows. The docs state it, and VERIFY names it as unmeasured." + evidence: '* key — on win32 `process.env` itself is case-insensitive, so a mixed-case key' +``` + +## Verdict + +**GREEN — 0 floor-gate findings, 2 advisory (1 important, 1 minor).** The standing decision is the +human's (GATE 2). The important one needs a maintainer's edit to `LIMITS.md` §3a. diff --git a/.dev/features/proxy-notice-truth/SHIP.md b/.dev/features/proxy-notice-truth/SHIP.md new file mode 100644 index 00000000..eca3b0ec --- /dev/null +++ b/.dev/features/proxy-notice-truth/SHIP.md @@ -0,0 +1,24 @@ +# SHIP — proxy-notice-truth + +Stages run, in order: + +1. `/pharn-dev-plan` → GATE 1 (human: plans A–F accepted with every recommended answer). +2. `/pharn-dev-grill`. +3. The plan's `## Files` was reworded so the writes-scope parser reads every path, and one measured + precedence detail was added: an empty `https_proxy` shadows `HTTPS_PROXY`. Intent unchanged. +4. `/pharn-dev-build` → `/pharn-dev-regress` → `/pharn-dev-verify` → `/pharn-dev-review` → GATE 2. + +| stage | structural verdict (verbatim) | +| -------------------- | ------------------------------------------------------ | +| `/pharn-dev-build` | `node .dev/floor/validate.mjs .` exit `0` | +| `/pharn-dev-regress` | `regression-report.json` `.verdict` = `no-regressions` | +| `/pharn-dev-verify` | `verify-report.json` `.verdict` = `PASS` | + +- Review: [`REVIEW.md`](REVIEW.md) · Grill (advisory): [`GRILL.md`](GRILL.md) +- For the human, outside this increment's reach: `LIMITS.md` §3a (human-only) still says a proxy + variable changes nothing. The notice cites it (REVIEW.md, important advisory finding). +- The run ended at **GATE 2**. The human's standing instruction for this batch: after each + increment, open a pull request and merge it once its checks are green, then start the next plan. + +chain ran; the named floor verdicts are as shown — this is NOT a judgment that the increment is good or +wise; that is the human's call at the post-review gate. diff --git a/.dev/features/proxy-notice-truth/VERIFY.md b/.dev/features/proxy-notice-truth/VERIFY.md new file mode 100644 index 00000000..0bd504e2 --- /dev/null +++ b/.dev/features/proxy-notice-truth/VERIFY.md @@ -0,0 +1,49 @@ +# VERIFY — proxy-notice-truth + +## FLOOR layer (owns the verdict) + +The gates ran over the whole repo with the feature present, on node 22, with the session proxy +variables unset. They ran as root **without** `CAP_DAC_OVERRIDE` / `CAP_DAC_READ_SEARCH` / +`CAP_FOWNER` (`setpriv`), the CI-equivalent of this root sandbox. + +| gate | exit | +| -------------- | ---- | +| `format:check` | 0 | +| `lint` | 0 | +| `lint:md` | 0 | +| `test` | 0 | +| `test:floor` | 0 | +| `typecheck` | 0 | +| `validate` | 0 | + +- `test` is vitest (1554 tests). It collects this increment's own `tests/proxy-env.test.ts` (one + case per measured row) and `tests/proxy-env-format.test.ts`. 21 of their cases failed against the + base code. +- `test:floor` is floor.yml's `node --test` run (754 tests). +- There is no `structural:*` gate: the increment ships no eval-actual pair. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs`, `failing_gates: []`). + +Outside the verdict, the hermetic setup (grill finding 3) was checked by running the WHOLE vitest +suite under the environments that used to break it: + +| extra environment | base (worktree, same tree as `f1e8b92`) | head | +| ------------------------------------------------ | --------------------------------------- | -------- | +| `NODE_USE_ENV_PROXY=1` | **15 failed** / 1530 | 0 / 1554 | +| `HTTPS_PROXY=http://proxy.internal:3128` | **1 failed** / 1530 | 0 / 1554 | +| `NODE_OPTIONS=--use-env-proxy` + `HTTPS_PROXY=…` | — | 0 / 1554 | + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}`. No verifiers are +registered, so the verdict rests on the floor gates only. + +Residual (P0/P7): verified = the named gates passed; this is NOT a guarantee of correctness beyond +what those gates check — verifier concerns are advisory help, not assurance. + +The rule table is exactly as good as its measurements: + +- It covers nine Node versions on Linux. +- A future release line is classified by the runtime's own flag list, not by the table. +- The win32 case-insensitivity comes from `process.env` and was not measured here. +- `NO_PROXY` is still only hedged in the "on" message, never evaluated (GATE 1 answer 2). diff --git a/.dev/features/proxy-notice-truth/regression-report.json b/.dev/features/proxy-notice-truth/regression-report.json new file mode 100644 index 00000000..453fe271 --- /dev/null +++ b/.dev/features/proxy-notice-truth/regression-report.json @@ -0,0 +1,26 @@ +{ + "base": "f1e8b92cfba1c59c19b1743cde23e77c54ac64bb", + "inside": [ + "CHANGELOG.md", + "docs/troubleshooting.md", + "src/lib/proxy-env-format.ts", + "src/lib/proxy-env.ts", + "tests/proxy-env-format.test.ts", + "tests/proxy-env.test.ts", + "tests/setup/hermetic-env.ts", + "vitest.config.ts" + ], + "outside_gates": { + "tests": { + "base": 0, + "head": 0 + }, + "validate": { + "base": 0, + "head": 0 + } + }, + "regressions": [], + "pre_existing": [], + "verdict": "no-regressions" +} diff --git a/.dev/features/proxy-notice-truth/verify-report.json b/.dev/features/proxy-notice-truth/verify-report.json new file mode 100644 index 00000000..03df9684 --- /dev/null +++ b/.dev/features/proxy-notice-truth/verify-report.json @@ -0,0 +1,18 @@ +{ + "feature": "proxy-notice-truth", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "test:floor": 0, + "typecheck": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { + "registered": 0, + "findings": [] + } +} diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index cec44edf..3ad6306a 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/signal-lock-release/SHIP.md" + ".dev/features/proxy-notice-truth/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-25T05:22:33.736Z" + "set_at": "2026-09-25T05:32:54.033Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 2559a685..aadd5d9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`pharn update` could stop re-checking a capability whose files were still out of date.** When pharn cannot read a capability upstream, `update` keeps it, skips its files, and moves the skills version on. Once pharn could read it again, the next run removed it from `frozenCapabilities` even if one of its files had to be skipped, for example because you edited it. Every later run then said "Already up to date", and that file stayed at the old version even after you resolved your edit. The capability now stays listed until none of its files are skipped, so each run checks it again. `update` also no longer writes a `pendingSkillsVersion` equal to the recorded `skillsVersion`. - **An interrupted run no longer leaves `.pharn.lock` behind.** The lock was released only on a normal return or a `process.exit`. A real signal ends the process without either, so `pharn update --yes` cancelled in CI, or stopped by `timeout` or `docker stop`, exited 130/143 with the lock still in the project. On the same machine the next run reclaimed it. A machine sharing the directory (a container bind mount) had to wait out the six-hour staleness window. `SIGINT` and `SIGTERM` now release the lock and remove the temp download first, print that the project may be partially updated, and still exit 130 or 143. A hangup (`SIGHUP`) is deliberately not handled: listening for it would override `nohup`, so a `nohup pharn update` would stop mid-write. - **A slow or failed GitHub API response no longer keeps `pharn` running after it has finished.** When the commit-SHA lookup got an error response (for example a 403 rate limit), or a success whose body was still arriving at the 8-second limit, the command carried on without the SHA as designed, but it left that response unread. The open connection kept the process alive until the server finished sending. It was measured at 30 seconds after `pharn status` had printed its last line. Such responses are now released at once. +- **The proxy notice now says what Node's `fetch` actually does.** It was wrong in several cases, each now measured on the Node releases that matter: + - On Node 24.0–24.4, `NODE_USE_ENV_PROXY` works but the notice said this Node had no support. + - `--use_env_proxy` (underscores) was not recognised. + - `--no-use-env-proxy` did not count as turning the opt-in off. + - A mixed-case name such as `Https_Proxy`, which Node never reads, was reported as the proxy in use. + - When both cases were set, the notice named `HTTPS_PROXY`, although Node uses `https_proxy`. + - With only `HTTP_PROXY` set, pharn said nothing, although Node sends https requests through it once the opt-in is on. + + Only the notice changes; the network behaviour is Node's and is unchanged. +- **The test suite no longer depends on the proxy settings of the machine running it.** With `NODE_USE_ENV_PROXY=1` set, 15 tests failed that pass in CI. With `HTTPS_PROXY` exported, 1 did. A setup file now clears the proxy variables before every test file. ### Security diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 04d7e965..17f79a9a 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -264,15 +264,30 @@ resolve, the repo tarball, and `SKILLS_VERSION` for `update` / `status --no-drif Node's global `fetch`, which by default reads **no** proxy environment variable: not `https_proxy`, not `HTTPS_PROXY`, not `no_proxy`. -**Recent Node versions can opt in.** Run `pharn` with `NODE_USE_ENV_PROXY=1` set (or pass -`--use-env-proxy` to Node, e.g. through `NODE_OPTIONS`) and Node's `fetch` routes through `HTTPS_PROXY` -and honours `NO_PROXY`. Older Nodes — Node 20, for example — have no such option. `pharn` checks what -the running Node supports (`process.allowedNodeEnvironmentFlags`) rather than guessing from a version -number. - -If a proxy variable is set, `pharn` says which of the three cases applies **before** it fetches, so a -network that blocks direct egress produces an explanation rather than an unexplained timeout. Without -the opt-in, on a Node that supports it: +**Recent Node versions can opt in.** On Node 22.21+ and 24.5+, run `pharn` with +`NODE_USE_ENV_PROXY=1` set, or pass `--use-env-proxy` to Node (for example through `NODE_OPTIONS`). +Node's `fetch` then routes through the proxy and honours `NO_PROXY`. The details, measured on each +release line: + +- **Only `NODE_USE_ENV_PROXY=1` counts** on those versions — `true` or `0` do not. +- **The flag wins over the variable, and the last flag wins.** `--no-use-env-proxy` turns the opt-in + off even when the variable is set; the command line is read after `NODE_OPTIONS`. Either `-` or `_` + works in the flag's name. +- **Node 24.0–24.4 have no flag yet**, but there **any** non-empty `NODE_USE_ENV_PROXY` turns it on. +- **Node 20, 21, 22.0–22.20 and 23 have no opt-in at all.** + +Which variable Node reads, for the https URLs `pharn` fetches: + +- **`https_proxy`, then `HTTPS_PROXY`**, and if neither gives a proxy, **`http_proxy`, then + `HTTP_PROXY`**. Lowercase wins when both cases are set. +- **An empty `https_proxy` hides `HTTPS_PROXY`**: Node stops at the first variable that is present + at all, and then falls through to the `http_proxy` pair. +- **Only those four exact spellings are read** (on Linux and macOS). A `Https_Proxy` is never used. + On Windows, environment names are case-insensitive, so any spelling works there. + +If a proxy variable is set, `pharn` says which case applies **before** it fetches, so a network that +blocks direct egress produces an explanation rather than an unexplained timeout. Without the +opt-in, on a Node that supports it: ```text ⚠ HTTPS_PROXY is set (http://***@proxy.internal:3128), but pharn will not use it: @@ -282,13 +297,14 @@ the opt-in, on a Node that supports it: the proxy — re-run with NODE_USE_ENV_PROXY=1 set. ``` -With the opt-in on, the notice instead says the downloads go through that proxy. On a Node without -the option it says so and that a newer Node release has it. +With the opt-in on, the notice instead says the downloads go through that proxy. After a +`--no-use-env-proxy`, it names that flag as what turned the proxy off. On a Node without the opt-in, +it says so and names the Node versions that have one. Two details in that message are deliberate: -- **It names the variable you actually set**, so a `Https_Proxy` typo shows up as read-and-still-unused - rather than as "pharn did not see it". +- **It names the variable Node would use**, and a spelling Node never reads (`Https_Proxy`) gets its + own notice, saying it is ignored and naming the four spellings that work — rather than silence. - **Credentials are redacted.** Any `user:password@` in the value is replaced with `***`. The value is only printed; it is never written to `pharn.config.json`, which lives in your repository and is committed. diff --git a/src/lib/proxy-env-format.ts b/src/lib/proxy-env-format.ts index 6647f5db..cad73ccb 100644 --- a/src/lib/proxy-env-format.ts +++ b/src/lib/proxy-env-format.ts @@ -42,20 +42,27 @@ export function redactProxyUrl(value: string): string { } /** - * The notice for a configured proxy, in one of three TRUE forms (PHARN-12). - * Node's fetch ignores proxy variables by default — which the old single - * message stated as absolute ("pharn will not use it") — but recent Node - * versions honour them when `NODE_USE_ENV_PROXY=1` / `--use-env-proxy` is set, - * so that message was false for users who had opted in and hid the one - * workaround from users who had not. + * The notice for a configured proxy, in one of its TRUE forms (PHARN-12, then + * re-measured): Node's fetch ignores proxy variables by default, honours them + * once the opt-in is on (on the Nodes that have one), and never reads a + * spelling other than the four exact ones. Each form says which of those + * applies to THIS run, and the one change that would alter it. */ export function proxyNoticeMessage(notice: ProxyNotice): string { const shown = `${notice.name} is set (${redactProxyUrl(notice.value)})`; if (notice.envProxy === 'on') { return `${shown} and NODE_USE_ENV_PROXY / --use-env-proxy is on, so pharn's downloads go through that proxy (Node's fetch then also honours NO_PROXY).`; } - const base = `${shown}, but pharn will not use it: its network calls go through Node's global fetch, which reads no proxy environment variable by default. The download connects DIRECTLY, and fails if direct egress is blocked (LIMITS.md §3a).`; - return notice.envProxy === 'available' - ? `${base} This Node can route fetch through the proxy — re-run with NODE_USE_ENV_PROXY=1 set.` - : `${base} This Node (${process.version}) has no NODE_USE_ENV_PROXY support; a newer Node release does.`; + const direct = + 'The download connects DIRECTLY, and fails if direct egress is blocked (LIMITS.md §3a).'; + if (notice.envProxy === 'ignored-spelling') { + return `${shown}, but pharn will not use it: Node reads only https_proxy, HTTPS_PROXY, http_proxy and HTTP_PROXY, spelled exactly so. ${direct}`; + } + const base = `${shown}, but pharn will not use it: its network calls go through Node's global fetch, which reads no proxy environment variable by default. ${direct}`; + if (notice.envProxy === 'available') { + return notice.optedOut + ? `${base} A --no-use-env-proxy flag (in NODE_OPTIONS or on the command line) turns Node's proxy support off — remove it to route fetch through the proxy.` + : `${base} This Node can route fetch through the proxy — re-run with NODE_USE_ENV_PROXY=1 set.`; + } + return `${base} This Node (${process.version}) cannot route fetch through a proxy; Node 22.21+ and 24+ can, with NODE_USE_ENV_PROXY=1.`; } diff --git a/src/lib/proxy-env.ts b/src/lib/proxy-env.ts index 9d51081e..6f1d7f7e 100644 --- a/src/lib/proxy-env.ts +++ b/src/lib/proxy-env.ts @@ -4,54 +4,61 @@ * wording reasons while this file changes only when the transport does (P3, * mirroring model-routing.ts / model-routing-format.ts). * - * The answer is now simple, and simpler than it used to be. pharn fetches - * everything — the SHA resolve, the repo tarball, `SKILLS_VERSION` — through - * Node's global `fetch`, and **Node's fetch does not read proxy environment - * variables at all.** Not `https_proxy`, not `HTTPS_PROXY`, not `no_proxy`, on - * any platform. So a user with a proxy configured is not partially proxied or - * proxied-on-one-platform: they are not proxied, and every pharn network call - * attempts direct egress. + * pharn fetches everything — the SHA resolve, the repo tarball, + * `SKILLS_VERSION` — through Node's global `fetch`. By DEFAULT that fetch reads + * no proxy environment variable at all, so a user with a proxy configured + * connects directly, and in a network that blocks direct egress the visible + * symptom is a timeout with nothing pointing at the cause (LIMITS.md §3a). + * Recent Nodes can opt in (`NODE_USE_ENV_PROXY=1` / `--use-env-proxy`), which + * is why the notice has to say, before the fetch, which case applies. * - * That is a real limit (LIMITS.md §3a), and it is worth a warning rather than a - * silent failure: in a network where direct egress is blocked, the visible - * symptom is a timeout with nothing pointing at the cause. Telling the user - * their proxy is configured and unused turns a mystery into a known limitation. + * Every rule below was MEASURED (PHARN-12's first version was inferred, and was + * wrong on four counts): a local proxy logging CONNECTs, a client `fetch`ing an + * https URL under `env -i`, on the official 20.20.2, 21.7.3, 22.20.0, 22.21.0, + * 22.22.2, 23.11.1, 24.0.0, 24.4.1 and 24.5.0 binaries. * - * HISTORY, because the shape of this module still reflects it: the previous - * clone path went through degit, which read `process.env.https_proxy` itself — - * only that lowercase spelling, and never `no_proxy`. So the notice used to - * classify (proxied vs. ignored-because-misspelled) and to gate its confidence - * on the installed degit version. With degit retired there is no dependency - * whose behavior needs measuring, and nothing reads any spelling, so the - * classification and the version gate are both gone. What remains is the - * detection and the redaction — the two parts that were about the USER's - * environment rather than the dependency's quirks. + * - WHICH VARIABLE: undici's own lookup, `https_proxy ?? HTTPS_PROXY`, then + * `http_proxy ?? HTTP_PROXY` for an https URL whose pair gave nothing. `??`, + * not "first non-empty": a PRESENT empty `https_proxy` shadows `HTTPS_PROXY`. + * Exact spellings only on POSIX — `Https_Proxy` is never read. + * - WHETHER IT IS USED: where Node knows `--use-env-proxy` (22.21+, 24.5+), the + * last `--use-env-proxy` / `--no-use-env-proxy` token decides (either `-` or + * `_`, NODE_OPTIONS first, then the command line; `=false` still turns it ON), + * and with no token only `NODE_USE_ENV_PROXY=1` does. On 24.0–24.4 the flag + * does not exist yet, but ANY non-empty `NODE_USE_ENV_PROXY` turns it on. + * Everywhere else (20, 21, 22.0–22.20, 23) nothing does. + * + * HISTORY: the clone once went through degit, which read `https_proxy` itself; + * with degit retired, Node's own rules are the only ones that matter. */ -/** The lowercase name every variant is compared against. */ -const LOWER = 'https_proxy'; -/** The uppercase spelling users reach for — preferred when several are set. */ -const UPPER = 'HTTPS_PROXY'; +/** The spellings Node reads, in undici's lookup order (https pair first). */ +const HTTPS_VARS = ['https_proxy', 'HTTPS_PROXY'] as const; +const HTTP_VARS = ['http_proxy', 'HTTP_PROXY'] as const; +const READ_SPELLINGS: ReadonlySet = new Set([ + ...HTTPS_VARS, + ...HTTP_VARS, +]); /** - * A proxy is configured and pharn will not use it. `name` is the variable - * actually found — safe to print by construction, since only a key whose - * lowercase equals `https_proxy` can reach it (one of 2^11 ASCII spellings; it - * cannot carry a control character). `value` is raw and MUST be rendered through - * `redactProxyUrl`, never echoed. + * A proxy variable is set. `name` is safe to print by construction: it is one + * of the four spellings Node reads, or a key that case-folds to one of them — + * ASCII letters and `_` only, so it cannot carry a control character. `value` + * is raw and MUST be rendered through `redactProxyUrl`, never echoed. */ export interface ProxyNotice { name: string; value: string; /** - * Whether Node's fetch will actually use the proxy (PHARN-12). Node's fetch - * reads no proxy variable BY DEFAULT, but recent Node versions route it - * through the proxy when `NODE_USE_ENV_PROXY=1` or `--use-env-proxy` is set: - * - `on` — supported and turned on: pharn's downloads use the proxy; - * - `available` — supported but off: setting `NODE_USE_ENV_PROXY=1` would; - * - `unsupported` — this Node has no such option (e.g. Node 20). + * Whether Node's fetch will actually use it: + * - `on` — this Node routes fetch through it: pharn's downloads do; + * - `available` — this Node could, but the opt-in is off; + * - `unsupported` — this Node has no opt-in at all; + * - `ignored-spelling` — Node never reads this spelling (e.g. `Https_Proxy`). */ - envProxy: 'on' | 'available' | 'unsupported'; + envProxy: 'on' | 'available' | 'unsupported' | 'ignored-spelling'; + /** `available` because a `--no-use-env-proxy` token turned it off. */ + optedOut?: true; } /** What the running Node supports and was started with — injectable for tests. */ @@ -60,37 +67,74 @@ export interface ProxyRuntime { supportsEnvProxy: boolean; /** Node's own flags (`process.execArgv`). */ execArgv: readonly string[]; + /** `process.versions.node`, e.g. `24.4.1`. */ + nodeVersion: string; } -const ENV_PROXY_FLAG = '--use-env-proxy'; - /** The running process, read once per call. */ export function currentProxyRuntime(): ProxyRuntime { return { // A membership test the runtime answers itself: Node lists every option it - // accepts, so there is no version table to keep up to date (P5). - supportsEnvProxy: process.allowedNodeEnvironmentFlags.has(ENV_PROXY_FLAG), + // accepts (P5). The version below is consulted ONLY where this says no. + supportsEnvProxy: + process.allowedNodeEnvironmentFlags.has('--use-env-proxy'), execArgv: process.execArgv, + nodeVersion: process.versions.node, }; } -/** `--use-env-proxy` as a whole token (bare or `=value`), never a substring. */ -function hasFlagToken(tokens: readonly string[]): boolean { - return tokens.some( - (t) => t === ENV_PROXY_FLAG || t.startsWith(`${ENV_PROXY_FLAG}=`), - ); +/** + * `--use-env-proxy` / `--no-use-env-proxy`, with `-` or `_`, as a WHOLE token + * (bare or `=value`), never a substring. Group 1 is set for the negation. + */ +const FLAG_RE = /^--(no[-_])?use[-_]env[-_]proxy(?:=.*)?$/; + +/** + * The opt-in's state from the flags alone: `true` / `false` from the LAST flag + * token, `null` when there is none. NODE_OPTIONS is read first and the command + * line after it, which is Node's own order. NODE_OPTIONS is split on + * whitespace; Node also honours double-quoted arguments there, which this does + * not model — a flag token never contains a space, so no real flag is misread. + */ +function lastFlag( + env: Record, + execArgv: readonly string[], +): boolean | null { + let state: boolean | null = null; + for (const token of [...(env.NODE_OPTIONS ?? '').split(/\s+/), ...execArgv]) { + const m = FLAG_RE.exec(token); + if (m) state = m[1] === undefined; + } + return state; +} + +/** + * 24.0–24.4: `NODE_USE_ENV_PROXY` already worked there, before the flag + * existed (24.5). The one version test in this file — only reached when the + * runtime does not list the flag, so a future Node that does is classified by + * the membership test, never by this window. + */ +function isEarly24(nodeVersion: string): boolean { + const [major, minor] = nodeVersion.split('.').map(Number); + return major === 24 && minor !== undefined && minor <= 4; } function envProxyState( env: Record, runtime: ProxyRuntime, -): ProxyNotice['envProxy'] { - if (!runtime.supportsEnvProxy) return 'unsupported'; - const on = - env.NODE_USE_ENV_PROXY === '1' || - hasFlagToken(runtime.execArgv) || - hasFlagToken((env.NODE_OPTIONS ?? '').split(/\s+/)); - return on ? 'on' : 'available'; +): Pick { + if (runtime.supportsEnvProxy) { + const flag = lastFlag(env, runtime.execArgv); + if (flag === false) return { envProxy: 'available', optedOut: true }; + return { + envProxy: + flag === true || env.NODE_USE_ENV_PROXY === '1' ? 'on' : 'available', + }; + } + if (isEarly24(runtime.nodeVersion)) { + return { envProxy: isSet(env.NODE_USE_ENV_PROXY) ? 'on' : 'available' }; + } + return { envProxy: 'unsupported' }; } /** Present AND non-empty. An empty string is not a proxy setting. */ @@ -98,43 +142,64 @@ function isSet(value: string | undefined): value is string { return value !== undefined && value !== ''; } +/** undici's `a ?? b`: the first PRESENT spelling, even if its value is empty. */ +function firstPresent( + env: Record, + names: readonly string[], +): { name: string; value: string } | null { + for (const name of names) { + const value = env[name]; + if (value !== undefined) return { name, value }; + } + return null; +} + /** - * Every key that case-insensitively spells `https_proxy` and has a non-empty - * value, in a DETERMINISTIC order (P5): `HTTPS_PROXY` first when present, then - * the rest sorted. Without the sort the answer would depend on env insertion - * order, which is not a property any caller should depend on. + * The variable Node's fetch would use for an https URL, or `null`. By exact + * key — on win32 `process.env` itself is case-insensitive, so a mixed-case key + * is found there, exactly as Node finds it. */ -function proxyVariantKeys(env: Record): string[] { - const keys = Object.keys(env) - .filter((key) => key.toLowerCase() === LOWER && isSet(env[key])) - .sort(); - return keys.includes(UPPER) - ? [UPPER, ...keys.filter((k) => k !== UPPER)] - : keys; +function readVariable( + env: Record, +): { name: string; value: string } | null { + const https = firstPresent(env, HTTPS_VARS); + if (https !== null && https.value !== '') return https; + const http = firstPresent(env, HTTP_VARS); + return http !== null && http.value !== '' ? http : null; +} + +/** + * A set key that only CASE-FOLDS to a spelling Node reads (`Https_Proxy`). + * Sorted, so several such keys give one deterministic answer (P5), never the + * env object's insertion order. + */ +function ignoredSpelling( + env: Record, +): { name: string; value: string } | null { + const name = Object.keys(env) + .filter( + (key) => + !READ_SPELLINGS.has(key) && + READ_SPELLINGS.has(key.toLowerCase()) && + isSet(env[key]), + ) + .sort()[0]; + return name === undefined ? null : { name, value: env[name]! }; } /** * Report a configured proxy, or `null` when none is set. * - * Deterministic presence test (P5) over `env`, with no classification and no - * I/O. Pure: `env` is a parameter, never read from `process.*`, so every row is - * exercisable from a test on any host. No `platform` argument any more — the - * old one existed only because Node's `process.env` is case-insensitive on - * win32 and degit's read was case-sensitive, a distinction that mattered when - * one spelling worked and another did not. None of them work now, so the - * platform cannot change the answer. - * - * The terminal case is `null` — SILENCE — the complete and correct answer when - * nothing is set, not a degraded fallback. + * Pure: `env` and `runtime` are parameters, never read from `process.*` here, so + * every measured row is exercisable from a test on any host. The terminal case + * is `null` — SILENCE — the complete and correct answer when nothing is set. */ export function detectProxyNotice( env: Record, runtime: ProxyRuntime = currentProxyRuntime(), ): ProxyNotice | null { - const name = proxyVariantKeys(env)[0]; - if (name === undefined) return null; - const value = env[name]; - return isSet(value) - ? { name, value, envProxy: envProxyState(env, runtime) } - : null; + const read = readVariable(env); + if (read !== null) return { ...read, ...envProxyState(env, runtime) }; + const ignored = ignoredSpelling(env); + return ignored === null ? null : { ...ignored, envProxy: 'ignored-spelling' }; } diff --git a/tests/proxy-env-format.test.ts b/tests/proxy-env-format.test.ts index a6cdbabd..3ac48314 100644 --- a/tests/proxy-env-format.test.ts +++ b/tests/proxy-env-format.test.ts @@ -147,9 +147,47 @@ describe('proxyNoticeMessage — the three env-proxy states (PHARN-12)', () => { expect(m).toContain('NODE_USE_ENV_PROXY=1'); }); - it('unsupported: says this Node cannot, and that a newer one can', () => { + it('unsupported: says this Node cannot, and names the Nodes that can', () => { const m = proxyNoticeMessage(notice('unsupported')); expect(m).toContain('will not use it'); - expect(m).toContain('no NODE_USE_ENV_PROXY support'); + expect(m).toContain('cannot route fetch through a proxy'); + // Measured: 22.21+ and every 24 release honour NODE_USE_ENV_PROXY. + expect(m).toContain('22.21'); + expect(m).toContain('24'); + }); + + // The user set NODE_USE_ENV_PROXY=1 and a --no-use-env-proxy wins over it: + // telling them to set the variable they already set would send them in a + // circle. The notice names the flag that turned it off instead. + it('available after an opt-out: names --no-use-env-proxy instead of the variable', () => { + const m = proxyNoticeMessage({ ...notice('available'), optedOut: true }); + expect(m).toContain('will not use it'); + expect(m).toContain('--no-use-env-proxy'); + expect(m).not.toContain('re-run with NODE_USE_ENV_PROXY=1'); + }); +}); + +describe('proxyNoticeMessage — a spelling Node never reads', () => { + const m = proxyNoticeMessage({ + name: 'Https_Proxy', + value: PROXY, + envProxy: 'ignored-spelling', + }); + + it('says it is not used and that the download goes direct', () => { + expect(m).toContain('Https_Proxy'); + expect(m).toContain('will not use it'); + expect(m).toContain('DIRECTLY'); + expect(m).not.toContain('go through that proxy'); + }); + + it('names the spellings Node does read', () => { + for (const name of [ + 'https_proxy', + 'HTTPS_PROXY', + 'http_proxy', + 'HTTP_PROXY', + ]) + expect(m).toContain(name); }); }); diff --git a/tests/proxy-env.test.ts b/tests/proxy-env.test.ts index f8cfa782..41b171e5 100644 --- a/tests/proxy-env.test.ts +++ b/tests/proxy-env.test.ts @@ -1,120 +1,255 @@ import { describe, expect, it } from 'vitest'; -import { detectProxyNotice } from '../src/lib/proxy-env.js'; +import { detectProxyNotice, type ProxyRuntime } from '../src/lib/proxy-env.js'; -// detectProxyNotice is pure over an injected env record — it never reads -// process.env — so every row here runs identically on any host. +// detectProxyNotice is pure over an injected env record and runtime — it never +// reads process.* — so every row here runs identically on any host. // -// The contract it encodes is now a flat one: pharn's network calls go through -// Node's global fetch, which reads NO proxy environment variable, on any -// platform. So the only question is "is a proxy configured?", and the answer no -// longer depends on the spelling or the OS. The previous version of this file -// pinned degit's lowercase-only read and the win32 case-insensitivity that made -// one spelling work and another not; with degit retired, none of them work, and -// those distinctions were deleted rather than kept as decoration. - -describe('detectProxyNotice', () => { +// Every row is a MEASUREMENT, not a reading of Node's docs: a local proxy that +// logs CONNECTs, a client `fetch`ing https://example.com under `env -i`, on the +// official 20.20.2, 21.7.3, 22.20.0, 22.21.0, 22.22.2, 23.11.1, 24.0.0, 24.4.1 +// and 24.5.0 binaries. The notice must say what that fetch actually does. + +/** A Node that knows --use-env-proxy (22.21+, 24.5+). */ +const MODERN: ProxyRuntime = { + supportsEnvProxy: true, + execArgv: [], + nodeVersion: '24.5.0', +}; +/** 24.0–24.4: the variable works, the flag does not exist yet. */ +const EARLY_24: ProxyRuntime = { + supportsEnvProxy: false, + execArgv: [], + nodeVersion: '24.4.1', +}; +/** 20.x, 21.x, 22.0–22.20, 23.x: nothing turns it on. */ +const NONE: ProxyRuntime = { + supportsEnvProxy: false, + execArgv: [], + nodeVersion: '20.20.2', +}; + +const P = 'http://proxy:3128'; + +describe('detectProxyNotice — which variable Node reads', () => { it('is silent when nothing is set', () => { - expect(detectProxyNotice({})).toBeNull(); + expect(detectProxyNotice({}, MODERN)).toBeNull(); }); it('is silent for an empty value — an empty string is not a setting', () => { - expect(detectProxyNotice({ https_proxy: '' })).toBeNull(); - expect(detectProxyNotice({ HTTPS_PROXY: '' })).toBeNull(); + expect(detectProxyNotice({ https_proxy: '' }, MODERN)).toBeNull(); + expect(detectProxyNotice({ HTTPS_PROXY: '' }, MODERN)).toBeNull(); }); - it.each([ - ['https_proxy', 'https_proxy'], - ['HTTPS_PROXY', 'HTTPS_PROXY'], - ['Https_Proxy', 'Https_Proxy'], - ['HTTPS_proxy', 'HTTPS_proxy'], - ])('reports %s — every spelling is equally unread', (name) => { + it.each(['https_proxy', 'HTTPS_PROXY', 'http_proxy', 'HTTP_PROXY'])( + 'reports %s, a spelling Node reads', + (name) => { + expect(detectProxyNotice({ [name]: P }, NONE)).toEqual({ + name, + value: P, + envProxy: 'unsupported', + }); + }, + ); + + // Measured: lowercase:18081 + UPPER:18082 → the CONNECT went to 18081. + it('names the lowercase variable when both cases are set — the one Node uses', () => { const notice = detectProxyNotice( - { [name]: 'http://proxy:3128' }, - { supportsEnvProxy: false, execArgv: [] }, + { https_proxy: 'http://lower:1', HTTPS_PROXY: 'http://upper:1' }, + MODERN, ); - expect(notice).toEqual({ - name, - value: 'http://proxy:3128', - envProxy: 'unsupported', + expect(notice).toMatchObject({ + name: 'https_proxy', + value: 'http://lower:1', }); }); - it('ignores unrelated variables', () => { + // Measured: with only HTTP_PROXY set and the opt-in on, an https fetch still + // went through it (undici falls back from the https agent to the http one). + it('reports HTTP_PROXY alone, because https requests fall back to it', () => { + expect(detectProxyNotice({ HTTP_PROXY: P }, MODERN)).toMatchObject({ + name: 'HTTP_PROXY', + envProxy: 'available', + }); + }); + + it('prefers the https pair to the http pair', () => { expect( - detectProxyNotice({ HTTP_PROXY: 'http://p:1', NO_PROXY: 'x', PATH: '/' }), + detectProxyNotice( + { HTTPS_PROXY: 'http://s:1', http_proxy: 'http://h:1' }, + MODERN, + ), + ).toMatchObject({ name: 'HTTPS_PROXY' }); + }); + + // Measured: https_proxy='' with HTTPS_PROXY set → DIRECT; with HTTP_PROXY set + // as well → through HTTP_PROXY. undici's lookup is `??`, so a PRESENT empty + // value shadows the next spelling rather than being skipped. + it('lets an empty https_proxy shadow HTTPS_PROXY, as Node does', () => { + expect( + detectProxyNotice({ https_proxy: '', HTTPS_PROXY: 'http://s:1' }, MODERN), ).toBeNull(); + expect( + detectProxyNotice( + { + https_proxy: '', + HTTPS_PROXY: 'http://s:1', + HTTP_PROXY: 'http://h:1', + }, + MODERN, + ), + ).toMatchObject({ name: 'HTTP_PROXY', value: 'http://h:1' }); }); - it('prefers HTTPS_PROXY when several spellings are set', () => { - // Deterministic (P5): without a fixed preference the answer would depend on - // the env object's key insertion order, which no caller should rely on. - const notice = detectProxyNotice({ - zttps_proxy: 'http://z:1', - https_proxy: 'http://lower:1', - HTTPS_PROXY: 'http://upper:1', - }); - expect(notice?.name).toBe('HTTPS_PROXY'); + it('ignores unrelated variables', () => { + expect(detectProxyNotice({ NO_PROXY: 'x', PATH: '/' }, MODERN)).toBeNull(); }); +}); - it('falls back to sorted order when HTTPS_PROXY is absent', () => { - const notice = detectProxyNotice({ - https_proxy: 'http://lower:1', - Https_Proxy: 'http://mixed:1', - }); - expect(notice?.name).toBe('Https_Proxy'); // 'H' sorts before 'h' +// Measured: `Https_Proxy` with the opt-in on → DIRECT. On POSIX Node reads the +// four exact spellings only. (On win32 `process.env` is case-insensitive, so the +// exact lookups find a mixed-case key there and this branch is never reached.) +describe('detectProxyNotice — a spelling Node never reads', () => { + it('reports it as ignored, never as proxied', () => { + expect( + detectProxyNotice({ Https_Proxy: P, NODE_USE_ENV_PROXY: '1' }, MODERN), + ).toEqual({ name: 'Https_Proxy', value: P, envProxy: 'ignored-spelling' }); }); - it('skips an empty variant in favour of a set one', () => { - const notice = detectProxyNotice({ - HTTPS_PROXY: '', - https_proxy: 'http://real:1', - }); - expect(notice?.name).toBe('https_proxy'); + it('stays silent about it when a spelling Node reads is also set', () => { + expect( + detectProxyNotice({ Https_Proxy: 'http://x:1', https_proxy: P }, MODERN), + ).toMatchObject({ name: 'https_proxy' }); + }); + + it('picks deterministically among several (sorted), whatever the insertion order', () => { + expect( + detectProxyNotice( + { hTTPS_PROXY: P, HTTPS_proxy: P, Http_Proxy: P }, + MODERN, + )?.name, + ).toBe('HTTPS_proxy'); }); }); -// PHARN-12: Node's fetch ignores proxy variables BY DEFAULT, but a Node that -// knows --use-env-proxy honours them once NODE_USE_ENV_PROXY=1 / the flag is -// set. The notice must say which of the three is true. -describe('detectProxyNotice — env-proxy opt-in (PHARN-12)', () => { - const env = { HTTPS_PROXY: 'http://proxy:3128' }; - const supported = { supportsEnvProxy: true, execArgv: [] as string[] }; +describe('detectProxyNotice — whether fetch uses it (22.21+ / 24.5+)', () => { + const env = { HTTPS_PROXY: P }; it.each([ ['NODE_USE_ENV_PROXY=1', { ...env, NODE_USE_ENV_PROXY: '1' }, [], 'on'], - ['--use-env-proxy in execArgv', env, ['--use-env-proxy'], 'on'], + // Measured: on 22.22.2 and 24.5.0 only the value `1` counts. [ - '--use-env-proxy in NODE_OPTIONS', - { ...env, NODE_OPTIONS: '--max-old-space-size=512 --use-env-proxy' }, + 'NODE_USE_ENV_PROXY=true', + { ...env, NODE_USE_ENV_PROXY: 'true' }, [], - 'on', + 'available', ], - ['nothing set', env, [], 'available'], [ 'NODE_USE_ENV_PROXY=0', { ...env, NODE_USE_ENV_PROXY: '0' }, [], 'available', ], + ['--use-env-proxy on the command line', env, ['--use-env-proxy'], 'on'], + // Node accepts `_` for `-` in option names; the old check knew only `-`. + ['--use_env_proxy on the command line', env, ['--use_env_proxy'], 'on'], + [ + '--use_env_proxy in NODE_OPTIONS', + { ...env, NODE_OPTIONS: '--max-old-space-size=512 --use_env_proxy' }, + [], + 'on', + ], + // Measured: `=false` still turned it ON. + ['--use-env-proxy=false', env, ['--use-env-proxy=false'], 'on'], + // Measured: the negation wins over the variable… + [ + 'NODE_USE_ENV_PROXY=1 + --no-use-env-proxy', + { ...env, NODE_USE_ENV_PROXY: '1' }, + ['--no-use-env-proxy'], + 'available', + ], + [ + 'NODE_USE_ENV_PROXY=1 + --no-use_env_proxy in NODE_OPTIONS', + { ...env, NODE_USE_ENV_PROXY: '1', NODE_OPTIONS: '--no-use_env_proxy' }, + [], + 'available', + ], + // …and the LAST token wins, the command line after NODE_OPTIONS. + [ + '--no-… then --use-…', + env, + ['--no-use-env-proxy', '--use-env-proxy'], + 'on', + ], + [ + '--use-… then --no-…', + env, + ['--use-env-proxy', '--no-use-env-proxy'], + 'available', + ], + [ + 'NODE_OPTIONS on, command line off', + { ...env, NODE_OPTIONS: '--use-env-proxy' }, + ['--no-use-env-proxy'], + 'available', + ], + [ + 'NODE_OPTIONS off, command line on', + { ...env, NODE_OPTIONS: '--no-use-env-proxy' }, + ['--use-env-proxy'], + 'on', + ], [ 'a longer flag that merely starts the same (token, not substring)', { ...env, NODE_OPTIONS: '--use-env-proxy-foo' }, [], 'available', ], + ['nothing set', env, [], 'available'], ])('%s → %s', (_label, e, execArgv, expected) => { expect( - detectProxyNotice(e as Record, { ...supported, execArgv }) + detectProxyNotice(e as Record, { ...MODERN, execArgv }) ?.envProxy, ).toBe(expected); }); - it('a Node without the option is `unsupported` whatever is set', () => { + it('marks a run the user opted out of, so the notice can say why', () => { expect( detectProxyNotice( - { ...env, NODE_USE_ENV_PROXY: '1', NODE_OPTIONS: '--use-env-proxy' }, - { supportsEnvProxy: false, execArgv: ['--use-env-proxy'] }, - )?.envProxy, - ).toBe('unsupported'); + { ...env, NODE_USE_ENV_PROXY: '1' }, + { ...MODERN, execArgv: ['--no-use-env-proxy'] }, + ), + ).toMatchObject({ envProxy: 'available', optedOut: true }); }); }); + +describe('detectProxyNotice — Nodes without the flag', () => { + const env = { HTTPS_PROXY: P }; + + // Measured on 24.0.0 and 24.4.1: the variable works there although + // --use-env-proxy does not exist yet — and ANY non-empty value turns it on. + it.each(['1', 'true', '0'])( + '24.0–24.4 with NODE_USE_ENV_PROXY=%s → on', + (value) => { + expect( + detectProxyNotice({ ...env, NODE_USE_ENV_PROXY: value }, EARLY_24) + ?.envProxy, + ).toBe('on'); + }, + ); + + it('24.0–24.4 without it → available (the variable would work)', () => { + expect(detectProxyNotice(env, EARLY_24)?.envProxy).toBe('available'); + }); + + it.each(['20.20.2', '21.7.3', '22.20.0', '23.11.1'])( + '%s → unsupported, whatever is set', + (nodeVersion) => { + expect( + detectProxyNotice( + { ...env, NODE_USE_ENV_PROXY: '1', NODE_OPTIONS: '--use-env-proxy' }, + { ...NONE, nodeVersion }, + )?.envProxy, + ).toBe('unsupported'); + }, + ); +}); diff --git a/tests/setup/hermetic-env.ts b/tests/setup/hermetic-env.ts new file mode 100644 index 00000000..1b37c7f9 --- /dev/null +++ b/tests/setup/hermetic-env.ts @@ -0,0 +1,25 @@ +// Runs before every test file (vitest.config.ts → setupFiles). +// +// The suite must not depend on the proxy environment of the machine running it. +// The commands print a proxy notice before they fetch, and what that notice says +// is derived from these variables. So a developer who had opted in to Node's +// env proxy (`NODE_USE_ENV_PROXY=1`), or merely exported `HTTPS_PROXY`, saw +// tests fail that pass in CI: 15 of them under `NODE_USE_ENV_PROXY=1`, and +// init's "prints nothing extra" under an exported `HTTPS_PROXY`. +// +// DELETED rather than stubbed: a test that stubs one of these calls +// `vi.unstubAllEnvs()` afterwards, which restores the value from before the +// stub. Deleting here first makes that value "absent" — never the host's. +// NODE_OPTIONS goes too, because `--use-env-proxy` inside it turns the opt-in on. +for (const name of [ + 'HTTPS_PROXY', + 'https_proxy', + 'HTTP_PROXY', + 'http_proxy', + 'NO_PROXY', + 'no_proxy', + 'NODE_USE_ENV_PROXY', + 'NODE_OPTIONS', +]) { + delete process.env[name]; +} diff --git a/vitest.config.ts b/vitest.config.ts index c4b29277..d20efbbe 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -4,6 +4,9 @@ export default defineConfig({ test: { environment: 'node', include: ['tests/**/*.test.ts'], + // Clears the host's proxy variables before every test file, so no result + // depends on the environment of the machine running the suite. + setupFiles: ['tests/setup/hermetic-env.ts'], coverage: { provider: 'v8', include: ['src/**/*.ts'],