diff --git a/.changeset/9013-ast-child-vitest-env-membership.md b/.changeset/9013-ast-child-vitest-env-membership.md new file mode 100644 index 0000000000..da8e4db924 --- /dev/null +++ b/.changeset/9013-ast-child-vitest-env-membership.md @@ -0,0 +1,22 @@ +--- +--- + +Judge the spawned-vitest child environment by what it IS rather than by whether +`childVitestEnv`'s name occurs in its text (objectui#9013). + +`scripts/__tests__/spawned-vitest-child-env-8616.test.ts` derived its population +of vitest spawns by AST and then judged each one with `env.includes('childVitestEnv')` +over the resolved `env:` text. A comment lives inside the declaration's span, so a +call site somebody explained instead of fixing passed the gate. Measured with two +probe files differing by one comment line and nothing else: without the helper's +name the gate refused the spawn at `1 failed | 4 passed`; with +`// childVitestEnv() would be the right thing to use here.` above the same +hand-rolled `{ ...process.env, CI: 'true' }` it accepted it at `5 passed`. + +The `env:` expression is now classified on nodes — a call to `childVitestEnv()`, or +an object literal spreading one, reached directly or through the name the spawn +passes — so a comment about the helper resolves to nothing. The population walk, +its pre-filter, the floor, the named member and the live `isAgent` probe are +untouched; the population is still four spawns and all four still pass. + +Test only; no package is released by this change. diff --git a/scripts/__tests__/spawned-vitest-child-env-8616.test.ts b/scripts/__tests__/spawned-vitest-child-env-8616.test.ts index 697cd0977d..72aac35ee1 100644 --- a/scripts/__tests__/spawned-vitest-child-env-8616.test.ts +++ b/scripts/__tests__/spawned-vitest-child-env-8616.test.ts @@ -51,7 +51,23 @@ import { childVitestEnv } from './helpers/child-vitest-env'; * 3. Spawns are found by AST with identifiers RESOLVED to their declarations, * because both halves of what is judged — which program is started, and * which env it is given — are naturally written as named constants. - * 4. ⭐ And the helper is MEASURED, not trusted: a real child process reports + * 4. ⭐ The environment is judged by what it IS, never by whether the + * helper's NAME occurs in its text. Measured for objectui#9013, while the + * judgement was `env.includes('childVitestEnv')`: two probe files differing + * by one COMMENT line and nothing else — `// childVitestEnv() would be the + * right thing to use here.` sitting above a hand-rolled + * `{ ...process.env, CI: 'true' }` — split `1 failed | 4 passed` from + * `5 passed`. A comment lives inside the declaration's span, so it was part + * of the text being compared, and the accepted spelling was exactly the one + * the sibling gate's header calls out as the thing to refuse: a call site + * somebody EXPLAINED instead of fixing. + * ⛔ Narrower than objectui#8712's hole one gate over, and ⛔ not the same + * defect. There, presence was the wrong test in BOTH directions because the + * goal was REMOVAL — a tree that SET the variable passed a gate that existed + * to remove it, and the idiomatic scrub was refused. Here presence of the + * helper IS the goal, so the only reachable hole is text that names it + * without calling it. ⭐ It was reachable. + * 5. ⭐ And the helper is MEASURED, not trusted: a real child process reports * `std-env`'s own `isAgent` back, once under the helper's env and once * under an env the helper produced and a caller then re-marked. A helper * that silently stopped scrubbing would pass every static check above. @@ -129,11 +145,84 @@ function namesAVitest(node: ts.CallExpression, source: ts.SourceFile): boolean { return tokens.some((t) => /vitest/i.test(t.replace(/vitest[.\-\w]*config[.\w]*/gi, ''))); } +/** The one shared spelling of the child environment (`helpers/child-vitest-env.ts`). */ +const HELPER = 'childVitestEnv'; + +/** + * What the `env:` an individual spawn passes IS — never what its text contains. + * + * `helper` — it IS a call to `childVitestEnv()`, or an object literal that + * SPREADS one (`{ ...childVitestEnv(), NO_COLOR: '1' }`), reached directly or + * through the name the spawn hands the child. + * `foreign` — there is an `env:` and it is not that: a hand-rolled copy of + * `process.env`, a scrub of some other key, or a comment ABOUT the helper. + * All three hand the child this container's agent markers. + * `absent` — no `env:` property at all, so the child inherits this worker's + * environment outright, markers included. + */ +type EnvVerdict = 'helper' | 'foreign' | 'absent'; + +/** `childVitestEnv(...)` — the helper, actually CALLED. */ +function callsHelper(node: ts.Node): boolean { + if (!ts.isCallExpression(node)) return false; + const callee = node.expression; + if (ts.isIdentifier(callee)) return callee.text === HELPER; + return ts.isPropertyAccessExpression(callee) && callee.name.text === HELPER; +} + +/** The initializer of a `const`/`let` named `name` in this file, as a NODE. */ +function declarationInitializer(source: ts.SourceFile, name: string): ts.Expression | null { + let found: ts.Expression | null = null; + const visit = (node: ts.Node): void => { + if ( + ts.isVariableDeclaration(node) && + ts.isIdentifier(node.name) && + node.name.text === name && + node.initializer !== undefined + ) { + found = node.initializer; + } + node.forEachChild(visit); + }; + visit(source); + return found; +} + +/** + * Where an `env:` expression's value COMES FROM. + * + * ⚠️ This is the judgement, and it is made on NODES. The text of the resolved + * declaration is never consulted, because a comment sitting inside that + * declaration's span is part of its text and answers "does this name the + * helper" exactly as well as a line calling it does (objectui#9013). + * + * ⛔ The resolution stays deliberately narrow — the declaration of the name the + * spawn passes, and the names spread into it, never the whole file. Anything + * wider answers "does this FILE mention `childVitestEnv`", which is the same + * question by a longer route. + */ +function envVerdict(value: ts.Expression, source: ts.SourceFile, seen: Set): EnvVerdict { + if (ts.isIdentifier(value)) { + if (seen.has(value.text)) return 'foreign'; + seen.add(value.text); + const initializer = declarationInitializer(source, value.text); + return initializer === null ? 'foreign' : envVerdict(initializer, source, seen); + } + if (callsHelper(value)) return 'helper'; + if (ts.isObjectLiteralExpression(value)) { + for (const property of value.properties) { + if (!ts.isSpreadAssignment(property)) continue; + if (envVerdict(property.expression, source, seen) === 'helper') return 'helper'; + } + } + return 'foreign'; +} + interface VitestSpawn { readonly file: string; readonly line: number; - /** Text of the `env:` value passed in the options object, if any — IDENTIFIER resolved. */ - readonly env: string | null; + /** What the `env:` this spawn passes IS — decided on the AST, never on text. */ + readonly verdict: EnvVerdict; } /** Every call in `file` that starts a child vitest. */ @@ -155,23 +244,23 @@ function vitestSpawns(file: string): VitestSpawn[] { : ''; if (SPAWNERS.has(callee) && namesAVitest(node, source)) { const options = node.arguments.find(ts.isObjectLiteralExpression.bind(ts)); - let env: string | null = null; + let verdict: EnvVerdict = 'absent'; if (options !== undefined) { for (const property of options.properties) { const key = property.name !== undefined && ts.isIdentifier(property.name) ? property.name.text : ''; if (key !== 'env') continue; if (ts.isShorthandPropertyAssignment(property)) { - env = declarationText(source, property.name.text) ?? property.name.text; + verdict = envVerdict(property.name, source, new Set()); } else if (ts.isPropertyAssignment(property)) { - env = resolved(property.initializer, source); + verdict = envVerdict(property.initializer, source, new Set()); } } } found.push({ file, line: source.getLineAndCharacterOfPosition(node.getStart(source)).line + 1, - env, + verdict, }); } } @@ -200,8 +289,13 @@ describe(`objectui#8616 — ${SPAWNS.length} vitest spawn(s) in the test tree`, }); it('every one of them builds the child environment with childVitestEnv()', () => { - const leaking = SPAWNS.filter((s) => s.env === null || !s.env.includes('childVitestEnv')).map( - (s) => `${s.file}:${s.line}`, + const leaking = SPAWNS.filter((s) => s.verdict !== 'helper').map( + (s) => + `${s.file}:${s.line} — ${ + s.verdict === 'absent' + ? 'no `env:` at all' + : 'this `env:` is not `childVitestEnv()` and does not spread one' + }`, ); expect( @@ -213,7 +307,9 @@ describe(`objectui#8616 — ${SPAWNS.length} vitest spawn(s) in the test tree`, 'byte stream CI never produces, and the assertion is verified against the wrong ' + 'thing forever (objectui#8616). ⛔ Imitating CI with `CI=true` does not expose ' + 'it, and neither does `FORCE_COLOR=1`. Use `childVitestEnv()` from ' + - '`scripts/__tests__/helpers/child-vitest-env.ts`:\n ' + + '`scripts/__tests__/helpers/child-vitest-env.ts`. ⚠️ NAMING the helper is not ' + + 'using it — this is read off the AST, so a comment about `childVitestEnv()` ' + + 'beside a hand-rolled environment resolves to nothing (objectui#9013):\n ' + leaking.join('\n '), ).toEqual([]); });