From 52a8b9fd1438f43d8f79be2bae7ff19117021001 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 15:58:17 +0000 Subject: [PATCH] test(cli): strip NODE_PATH in childEnv() so a spawned CJS resolution measures its real base A vitest worker runs with NODE_PATH pointing at pnpm's hoisted store, which holds everything transitively reachable anywhere in the workspace. childEnv() stripped only the vitest worker family, so NODE_PATH rode into every spawned child. CJS createRequire() honours it (ESM import() does not), and it is a FALLBACK rather than an override -- so the store can only turn a MISS into a HIT. That makes an ACCEPTANCE claim ("this base CAN reach X") green because the store supplied X, not because the base did: spawning a real Node child escapes Vite's rewrite but not this, so a spawned resolution pin routed through CJS was as vacuous as the in-process one it replaced. childEnv() now strips NODE_PATH too, under its own named family (RESOLUTION_BASE_ENV_KEYS) rather than folded into VITEST_WORKER_ENV_KEYS: NODE_PATH is not something vitest sets, every pnpm bin shim exports one, and a real serve/dev child in production carries it. Overrides still apply after the strip, so a test reproducing the shim shape asks for it explicitly -- which the #4719 pin already does. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HbG3rGVLjZStHQxHDtzJdJ --- packages/cli/test/helpers/serve-process.ts | 61 +++++++++-- ...itest-resolution-base-collapse.e2e.test.ts | 101 ++++++++++++++---- scripts/check-test-source-alias.mjs | 9 +- 3 files changed, 140 insertions(+), 31 deletions(-) diff --git a/packages/cli/test/helpers/serve-process.ts b/packages/cli/test/helpers/serve-process.ts index df853e5a93..765605d110 100644 --- a/packages/cli/test/helpers/serve-process.ts +++ b/packages/cli/test/helpers/serve-process.ts @@ -135,15 +135,62 @@ function isVitestWorkerKey(key: string): boolean { return key === 'TEST' || key === 'VITEST' || key.startsWith('VITEST_'); } +/** + * Variables that silently move a spawned child's module RESOLUTION BASE. A + * different defect from the runner leak above, stripped for a different reason + * (#11773). + * + * A vitest worker runs with `NODE_PATH` pointing at pnpm's hoisted store + * (`node_modules/.pnpm/node_modules`), which holds everything transitively + * reachable anywhere in the workspace. `NODE_PATH` is a FALLBACK, not an + * override — the `node_modules` walk wins whenever it hits — so the store can + * only turn a MISS into a HIT. The dangerous direction is therefore an + * ACCEPTANCE claim ("this base CAN reach X"): green because the store supplied + * X, not because the base did. + * + * The split that decides whether it bites, measured in + * `test/vitest-resolution-base-collapse.e2e.test.ts`: + * + * resolution API NODE_PATH honoured? base kept? + * ESM import() / import.meta.resolve NO YES + * CJS createRequire().resolve() YES NO + * + * Spawning a real Node child is this directory's remedy for the resolution base + * an in-process test cannot measure at all (#11412) — it escapes Vite's + * rewrite, but it did NOT escape this, so a spawned pin whose claim routes + * through CJS was as vacuous as the in-process one it replaced. + * `serve-host-fallback-base.e2e.test.ts`'s control survived only because + * `createHostImporter`'s fallback leg happens to be an ESM `import()`; had it + * been CJS — as `createHostRequire` is — the inherited `NODE_PATH` would have + * kept it green through the very ablation it exists to fail. + * + * ⛔ Deliberately NOT folded into `VITEST_WORKER_ENV_KEYS` above. That list is + * "what vitest sets on its own worker", and `NODE_PATH` is not vitest's: every + * pnpm bin shim exports one, so a REAL `serve`/`dev` child in production + * carries it — that is #4719's entire history. Stripping it here is a DEFAULT + * for spawned test children, not a claim that no child should ever see it. A + * test that reproduces the shim shape says so explicitly, and the #4719 pin in + * `serve-organizations-host-resolution.e2e.test.ts` already does + * (`env: { …, NODE_PATH: hoistedStore }`) — `overrides` are applied after the + * strip, so that opt-in still wins. What changes is that the fidelity is now + * DECLARED by the test that wants it rather than inherited by every child. + */ +export const RESOLUTION_BASE_ENV_KEYS = ['NODE_PATH'] as const; + +const RESOLUTION_BASE_ENV_SET: ReadonlySet = new Set(RESOLUTION_BASE_ENV_KEYS); + /** * Build the environment for a spawned CLI child: this process's environment - * minus the vitest worker family above, plus `overrides`. + * minus the TWO families stripped above, plus `overrides`. * - * The strip is a **class**, not the fixed list: `TEST` exactly, plus anything - * matching `VITEST`/`VITEST_*`. `VITEST_WORKER_ENV_KEYS` names the five that - * vitest 4 exports today (and is what the pin asserts against), but a future - * runner variable in that namespace is caught without anyone having to - * rediscover this trap first. + * The vitest-worker strip is a **class**, not the fixed list: `TEST` exactly, + * plus anything matching `VITEST`/`VITEST_*`. `VITEST_WORKER_ENV_KEYS` names + * the five that vitest 4 exports today (and is what the pin asserts against), + * but a future runner variable in that namespace is caught without anyone + * having to rediscover this trap first. The resolution-base strip + * (`RESOLUTION_BASE_ENV_KEYS`) is the opposite shape — exact names only, no + * namespace — because `NODE_PATH` is a variable real children legitimately + * carry, so widening it by prefix would strip things nobody measured. * * `overrides` is applied AFTER the strip, so a test that genuinely wants one of * these set in its child can still say so explicitly — the point is that @@ -156,7 +203,7 @@ export function childEnv( ): Record { const env: Record = {}; for (const [key, value] of Object.entries(process.env)) { - if (isVitestWorkerKey(key)) continue; + if (isVitestWorkerKey(key) || RESOLUTION_BASE_ENV_SET.has(key)) continue; env[key] = value; } return { ...env, ...overrides }; diff --git a/packages/cli/test/vitest-resolution-base-collapse.e2e.test.ts b/packages/cli/test/vitest-resolution-base-collapse.e2e.test.ts index c9f3c10750..ba536a8852 100644 --- a/packages/cli/test/vitest-resolution-base-collapse.e2e.test.ts +++ b/packages/cli/test/vitest-resolution-base-collapse.e2e.test.ts @@ -33,13 +33,13 @@ * spawned-child pin of the same claim in `serve-host-fallback-base.e2e.test.ts` * went RED. Same tree, same ablation, opposite verdicts. * - * **M2 — `NODE_PATH` reaches the spawned child, and CJS honours it.** This one - * is NOT vitest rewriting anything, and it survives the obvious remedy. A vitest + * **M2 — `NODE_PATH` reaches a spawned child, and CJS honours it.** This one is + * NOT vitest rewriting anything, and it survives the obvious remedy. A vitest * worker runs with `NODE_PATH` pointing at pnpm's hoisted store * (`node_modules/.pnpm/node_modules`, which holds everything transitively - * reachable in the workspace), and `test/helpers/serve-process.ts`'s `childEnv()` - * strips only `TEST` / `VITEST*` — so `NODE_PATH` rides into every spawned child - * this package starts. + * reachable in the workspace), and until #11773 + * `test/helpers/serve-process.ts`'s `childEnv()` stripped only `TEST` / + * `VITEST*` — so `NODE_PATH` rode into every spawned child this package starts. * * The split that decides whether that matters, measured here: * @@ -56,8 +56,23 @@ * child escapes M1 but NOT M2. `serve-host-fallback-base.e2e.test.ts`'s CONTROL * is sound only because `createHostImporter`'s fallback leg is an ESM `import()`. * Had it been CJS — as `createHostRequire` is — the inherited `NODE_PATH` would - * have kept it green through the very ablation it exists to fail. A spawned pin - * whose claim routes through CJS must pass `childEnv({ NODE_PATH: undefined })`. + * have kept it green through the very ablation it exists to fail. + * + * ── The repair (#11773), and what it moved in this file ──────────────────── + * + * `childEnv()` now strips `NODE_PATH` too (`RESOLUTION_BASE_ENV_KEYS`), so M2 is + * closed by DEFAULT rather than by every author of a spawned pin remembering it. + * The store is still one override away — `childEnv({ NODE_PATH: … })` — which is + * how the #4719 pin in `serve-organizations-host-resolution.e2e.test.ts` + * reproduces the pnpm bin shim on purpose, and how the `withStore` leg below + * still measures the platform split M2 is about. ⛔ Do not delete that leg: the + * `clean` readings are only a measurement while something in this file can still + * make the probe answer the other way. + * + * The last describe block is the pin the strip exists for. It is deliberately + * NOT "assert `NODE_PATH` is absent from the child env" — the defect being fixed + * here WAS a vacuous pin, and an absence assertion repeats that failure one + * level up by passing against a child that resolves nothing at all. * * ── Reading this file ────────────────────────────────────────────────────── * @@ -132,17 +147,27 @@ console.log(JSON.stringify({ esmCliDeclared: attempt(() => import.meta.resolve(${JSON.stringify(CLI_DECLARED)})), esmTypesDeclared: attempt(() => import.meta.resolve(${JSON.stringify(TYPES_DECLARED)})), esmNowhere: attempt(() => import.meta.resolve(${JSON.stringify(NOWHERE)})), + // PROOF OF ANCHOR for the CJS leg. \`esmReferrer\` above proves it for ESM; + // these are two different resolvers and neither one's anchor is evidence for + // the other's — which is the whole subject of this file. + cjsFirstSearchPath: (req.resolve.paths(${JSON.stringify(CLI_DECLARED)}) ?? [])[0] ?? '(none)', cjsCliDeclared: attempt(() => req.resolve(${JSON.stringify(CLI_DECLARED)})), + cjsTypesDeclared: attempt(() => req.resolve(${JSON.stringify(TYPES_DECLARED)})), cjsNowhere: attempt(() => req.resolve(${JSON.stringify(NOWHERE)})), noBaseImporter: await attemptAsync(() => createHostImporter(appRoot)(${JSON.stringify(CLI_DECLARED)})), })); `; let appRoot: string; -/** Real Node, `NODE_PATH` stripped — the uncontaminated baseline. */ +/** Real Node, the env `childEnv()` hands over TODAY — `NODE_PATH` stripped (#11773). */ let clean: Record; -/** Real Node, `NODE_PATH` exactly as `childEnv()` hands it over. */ -let inherited: Record; +/** + * The same child with the hoisted store put back on `NODE_PATH` EXPLICITLY — + * the pnpm bin shim shape, and since #11773 the only way a child gets it. This + * leg is what keeps the readings above from being a claim about a child that + * simply cannot resolve anything. + */ +let withStore: Record; async function runChild(env: Record): Promise> { const { stdout } = await execFileAsync( @@ -163,8 +188,8 @@ beforeAll(async () => { join(appRoot, 'package.json'), JSON.stringify({ name: 'fixture-app', version: '1.0.0', type: 'module' }), ); - clean = await runChild(childEnv({ NO_COLOR: '1', NODE_PATH: undefined })); - inherited = await runChild(childEnv({ NO_COLOR: '1' })); + clean = await runChild(childEnv({ NO_COLOR: '1' })); + withStore = await runChild(childEnv({ NO_COLOR: '1', NODE_PATH: process.env.NODE_PATH })); }, 120_000); afterAll(() => { @@ -184,7 +209,7 @@ describe('#11412 CONTROLS — the probe can return every answer it is asked to d }); it('CAN say MISS: a specifier nothing satisfies misses on both legs, both envs', () => { - for (const probe of [clean, inherited]) { + for (const probe of [clean, withStore]) { expect(probe.esmNowhere).toMatch(/^MISS:/); expect(probe.cjsNowhere).toMatch(/^MISS:/); } @@ -215,21 +240,55 @@ describe('#11412 M1 — vitest flattens the resolution base an in-process test w }); }); -describe('#11412 M2 — spawning escapes Vite, but NODE_PATH rides along and CJS honours it', () => { - it('childEnv() hands NODE_PATH to the child (it strips only TEST / VITEST*)', () => { - expect(inherited.nodePathSeen).not.toBe('(unset)'); - // The stripped-env leg is what proves the line above is about `childEnv()`'s - // policy and not about this box always having NODE_PATH set. +describe('#11412 M2 — NODE_PATH moves a child’s CJS base; #11773 stops it arriving by accident', () => { + it('the worker HAS a NODE_PATH, and childEnv() no longer hands it to the child', () => { + // PRECONDITION, in the sense this file's header uses the word. With no + // NODE_PATH on the worker there would be no store to strip, and every + // reading below would be green against nothing. + expect(process.env.NODE_PATH ?? '(unset)').not.toBe('(unset)'); expect(clean.nodePathSeen).toBe('(unset)'); + // …and an explicit override still wins, which is what the #4719 pin needs. + expect(withStore.nodePathSeen).not.toBe('(unset)'); }); - it('ESM ignores NODE_PATH: the base survives into the child', () => { + it('ESM ignores NODE_PATH: the base survives into the child either way', () => { expect(clean.esmCliDeclared).toMatch(/^MISS:/); - expect(inherited.esmCliDeclared).toMatch(/^MISS:/); + expect(withStore.esmCliDeclared).toMatch(/^MISS:/); }); it('CJS honours NODE_PATH: the same base, the same specifier, the opposite answer', () => { expect(clean.cjsCliDeclared).toMatch(/^MISS:/); - expect(inherited.cjsCliDeclared).toMatch(/^RESOLVED:/); + expect(withStore.cjsCliDeclared).toMatch(/^RESOLVED:/); + // The HIT names the store it came from, so the acceptance is provably the + // fallback supplying `chalk` rather than the base having reached it. + expect(withStore.cjsCliDeclared).toContain('.pnpm'); + }); + + it('FALLBACK, not override: the store cannot change an answer the walk already hits', () => { + // This is why only the ACCEPTANCE direction is dangerous — the store adds + // reachability, it never redirects it. + expect(clean.cjsTypesDeclared).toMatch(/^RESOLVED:/); + expect(withStore.cjsTypesDeclared).toBe(clean.cjsTypesDeclared); + }); +}); + +describe('#11773 — a CJS-routed resolution in a childEnv() child measures its REAL base', () => { + it('ANCHOR: the child’s CJS walk starts at the types package, not at the test', () => { + expect(clean.cjsFirstSearchPath).toBe(join(TYPES_BASE, 'node_modules')); + }); + + it('POSITIVE: that walk resolves — the base reaches its own declared dependency', () => { + // Prove the instrument produces a positive before trusting its negative: a + // probe that could only ever say MISS would "confirm" the line below while + // measuring nothing at all. + expect(clean.cjsTypesDeclared).toMatch(/^RESOLVED:/); + }); + + it('NEGATIVE: and it cannot reach a package its base does not declare', () => { + // The load-bearing assertion of this card. Before #11773 this read + // RESOLVED — the inherited hoisted store supplied `chalk`, and a spawned + // pin written over it would have certified a base that never reached it. + // Restore the forwarding in `childEnv()` and this line goes red. + expect(clean.cjsCliDeclared).toMatch(/^MISS:/); }); }); diff --git a/scripts/check-test-source-alias.mjs b/scripts/check-test-source-alias.mjs index 746e7d6b82..832e674d09 100644 --- a/scripts/check-test-source-alias.mjs +++ b/scripts/check-test-source-alias.mjs @@ -100,9 +100,12 @@ // // A pin that must measure a resolution BASE therefore spawns a real Node child. // ⚠️ Spawning escapes Vite but NOT `NODE_PATH`: a vitest worker carries pnpm's -// hoisted store in it, `childEnv()` forwards it, and CJS `createRequire()` -// honours it while ESM `import()` ignores it. A spawned pin whose claim routes -// through CJS must strip it (`childEnv({ NODE_PATH: undefined })`). +// hoisted store in it, and CJS `createRequire()` honours it while ESM +// `import()` ignores it. `childEnv()` forwarded it until #11773 and STRIPS it +// now (`RESOLUTION_BASE_ENV_KEYS`), so a spawned pin whose claim routes through +// CJS gets an honest base by default. A test that wants the pnpm bin shim shape +// back asks for it explicitly (`childEnv({ NODE_PATH: ... })`) -- which is what +// the #4719 pin does. // // The mechanism, both halves, and the controls that prove each one can fail live // in `packages/cli/test/vitest-resolution-base-collapse.e2e.test.ts`.