From 8b1a9c6db7a61b96e53006b297efe2bbd7ba603b Mon Sep 17 00:00:00 2001 From: Litant Ying Date: Wed, 26 Aug 2026 09:29:33 +0000 Subject: [PATCH] test(cli): name a callback arrow's site after its call, so sibling it() blocks key apart `enclosingFunctionName()` recognised four shapes -- a function declaration, a method, and an arrow/function expression bound to a variable or a property -- and none of them is the one that dominates a test directory: an arrow passed DIRECTLY as a call argument. `it('...', () => {})`, `beforeAll(() => {})` and `describe(...)` matched none, so the walk ran past the callback to the source file and attributed the site to `(top-level)`. That was never merely an ugly label. `siteKey(row)` is `file::fn` and the DELIBERATE registry is keyed by it, so two deliberate bulk copies in two `it()` blocks of ONE file both keyed to `::(top-level)` -- one entry would have silenced BOTH, and the second would never have been reviewed as part of the PR that needed it. That is the carve-out-by-accident shape the gate's own header says the registry exists to prevent. Latent, not live: both current DELIBERATE entries name real functions (`childEnv`, `leakedEnv`), so nothing was mis-keyed and the live census is byte-identical across this change. What is closed is the NEXT entry's problem. An arrow whose parent is a call with an IDENTIFIER callee is now named after that callee plus its first string-literal argument, falling back to the callee alone when there is no literal. Rule 2 reads the same walk and has no registry to mis-key, so it gets the better site in its MESSAGES from the same change. `audit()` takes an optional registry, defaulting to DELIBERATE. The property under test -- ONE entry silences ONE site -- cannot be asserted against a live registry whose two entries sit in two different files, so the self-test synthesises one and drives the real classification path with it. Self-test: 78 -> 92 cases. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- scripts/check-cli-test-child-env.mjs | 166 ++++++++++++++++++++++++++- 1 file changed, 163 insertions(+), 3 deletions(-) diff --git a/scripts/check-cli-test-child-env.mjs b/scripts/check-cli-test-child-env.mjs index 96db2d2fb2..cc860725a3 100644 --- a/scripts/check-cli-test-child-env.mjs +++ b/scripts/check-cli-test-child-env.mjs @@ -390,12 +390,67 @@ function isBulkUse(node) { return true; } +/** + * The name a call-argument arrow takes from the call it is passed to: + * `it("boots the child")`, or `beforeAll` when the call carries no title. + * + * DOUBLE quotes around the title, deliberately: the product is a + * {@link DELIBERATE} key, and this file's string literals are single-quoted, so + * a key pasted into that registry needs no escaping. + */ +function callbackSiteName(owner) { + if (!owner || !ts.isCallExpression(owner) || !ts.isIdentifier(owner.expression)) return null; + const callee = owner.expression.text; + const title = owner.arguments.find((arg) => ts.isStringLiteralLike(arg)); + return title ? `${callee}("${title.text.replace(/\s+/g, ' ').trim()}")` : callee; +} + /** * The nearest enclosing named function, which is the stable half of a site key. * * A line number is not: any edit above a site moves it, so a registry keyed by * line would go stale on unrelated changes and teach its readers to re-run * `--update` without looking. + * + * ## Why a call-argument arrow is named after its CALL (#12531) + * + * The four shapes below -- a function declaration, a method, and an + * arrow/function expression bound to a variable or to a property -- miss the + * one that dominates a TEST directory: an arrow passed DIRECTLY as a call + * argument. `it('boots the child', async () => { ... })`, `beforeAll(async () => + * { ... })` and `describe(...)` match none of them, so the walk ran straight + * past the callback to the source file and attributed the site to + * `(top-level)`. + * + * That was never merely an ugly label. {@link siteKey} is `file::fn` and + * {@link DELIBERATE} is keyed by it, so two deliberate bulk copies in two + * `it()` blocks of ONE file both keyed to `::(top-level)` -- and a single + * registry entry would have silenced BOTH, leaving the second unreviewed by the + * PR that needed it. That is exactly the carve-out-by-accident shape this + * file's header says the registry exists to prevent. ⚠️ It was LATENT and not + * live when it was found: both entries in the registry name real functions + * (`childEnv`, `leakedEnv`), so nothing was ever mis-keyed. What is closed here + * is the NEXT entry's problem. + * + * So an arrow whose parent is a call with an IDENTIFIER callee is named after + * that callee plus its first string-literal argument, falling back to the + * callee alone when there is no literal. Sibling blocks in one file therefore + * key differently, which is the whole repair -- and rule 2, which has no + * registry to mis-key, gets the same site in its MESSAGES for free. + * + * ⚠️ Three limits, named here so they are pinned rather than discovered: + * + * 1. The callee must be an IDENTIFIER. `it.skip('...')` and + * `it.each(table)('...')` call through a property access or through + * another call, and still walk past to `(top-level)`. That is a choice: + * widening to a property-access callee would also capture + * `promise.then(() => ...)` and `rows.map(() => ...)`, whose callee is a + * WORSE site name than the enclosing test block the walk reaches today. + * 2. The title's whitespace is collapsed so the name stays one line, but it + * is ⛔ never truncated -- two long titles sharing a prefix would collide + * again, which is the defect being closed. + * 3. The nearest NAMED binding still wins, unchanged: a helper declared + * inside an `it()` block is named after the helper, not after the block. */ export function enclosingFunctionName(node, sourceFile) { let cursor = node.parent; @@ -407,6 +462,8 @@ export function enclosingFunctionName(node, sourceFile) { if (owner && ts.isVariableDeclaration(owner) && owner.name) return owner.name.getText(sourceFile); if (owner && ts.isPropertyAssignment(owner) && owner.name) return owner.name.getText(sourceFile); if (cursor.name) return cursor.name.text; + const called = callbackSiteName(owner); + if (called) return called; } cursor = cursor.parent; } @@ -658,8 +715,16 @@ export const siteKey = (row) => `${row.file}::${row.fn}`; * never an empty finding list standing in for a population it could not read. * * @param {string} root A directory containing the population root. + * @param {Record} registry The declaration registry to + * classify against. Defaults to {@link DELIBERATE}, which is what every + * caller but the self-test uses. The parameter exists because the property + * the site key has to hold -- that ONE entry silences ONE site -- cannot be + * asserted against the live registry, whose two entries sit in two different + * files and name real functions. A synthesised registry is what lets the + * #12531 collision be shown GONE through the real classification path, + * rather than read off a key string. */ -export function audit(root) { +export function audit(root, registry = DELIBERATE) { const populationDir = join(root, POPULATION_ROOT); let exists = false; try { @@ -705,7 +770,7 @@ export function audit(root) { spawners += 1; for (const hit of bulkEnvReferences(abs, source)) { const row = { file: rel, ...hit }; - (Object.hasOwn(DELIBERATE, siteKey(row)) ? deliberate : findings).push(row); + (Object.hasOwn(registry, siteKey(row)) ? deliberate : findings).push(row); } const calls = envlessSpawnCalls(abs, source); spawnCalls += calls.calls; @@ -980,6 +1045,13 @@ export function selfTest() { const spawner = (body) => `import { execFile } from 'node:child_process';\n\nexport function runCli(cwd: string) {\n${body}\n}\n`; /** The same body in a file that imports nothing that spawns. */ const plain = (body) => `export function helper(cwd: string) {\n void cwd;\n${body}\n}\n`; + /** + * A spawner file whose body sits at TOP LEVEL rather than inside a named + * function -- the shape a real test file has, and the one the site-naming + * cases below need: wrapping them in `runCli` would name every site `runCli` + * and measure nothing about the walk. + */ + const suite = (body) => `import { execFile } from 'node:child_process';\nvoid execFile;\n${body}\n`; /** Findings from a one-file spawner tree. */ const scan = (name, sources) => { @@ -1104,6 +1176,85 @@ export function selfTest() { carvedNeighbour.findings?.length === 1 && carvedNeighbour.findings[0].fn === 'somethingElse', JSON.stringify(carvedNeighbour.findings)); + // -- (8b) SITE NAMING (#12531): a callback arrow is named after its CALL + // An arrow passed DIRECTLY as a call argument matches none of the four + // shapes the walk recognises, so before this it ran past `it(...)` and + // `beforeAll(...)` to the source file and reported `(top-level)`. + + /** The site names a one-file spawner tree attributes its bulk copies to. */ + const siteNames = (name, sources) => JSON.stringify(scan(name, sources).findings?.map((row) => row.fn)); + const names = (...expected) => JSON.stringify(expected); + const BULK = ' const e = { ...process.env };\n void e;'; + + t('a bulk copy inside an it() block is named after the BLOCK, not (top-level)', + siteNames('site-it', { 'a.e2e.test.ts': suite(`it('boots the child', () => {\n${BULK}\n});`) }) + === names('it("boots the child")')); + t('a callback with no string-literal title falls back to the callee alone', + siteNames('site-beforeall', { 'a.e2e.test.ts': suite(`beforeAll(async () => {\n${BULK}\n});`) }) + === names('beforeAll')); + + // ⭐ THE case. Two sibling blocks in ONE file must not share a key. + t('two it() blocks in one file get TWO DISTINCT names -- the collision this closes', + siteNames('site-siblings', { + 'a.e2e.test.ts': suite(`it('first', () => {\n${BULK}\n});\nit('second', () => {\n${BULK}\n});`), + }) === names('it("first")', 'it("second")')); + + t('the NEAREST block wins inside a describe()', + siteNames('site-nested', { + 'a.e2e.test.ts': suite(`describe('outer', () => {\n it('inner', () => {\n${BULK}\n });\n});`), + }) === names('it("inner")')); + t('a named helper declared inside an it() block still wins over the block', + siteNames('site-helper', { + 'a.e2e.test.ts': suite(`it('x', () => {\n function build() {\n return { ...process.env };\n }\n void build;\n});`), + }) === names('build')); + t('a variable-bound arrow inside an it() block still wins too', + siteNames('site-arrow-binding', { + 'a.e2e.test.ts': suite(`it('x', () => {\n const build = () => ({ ...process.env });\n void build;\n});`), + }) === names('build')); + t('a bulk copy at TOP LEVEL is still (top-level) -- the fallback survives', + siteNames('site-top', { 'a.e2e.test.ts': suite('export const e = { ...process.env };') }) + === names('(top-level)')); + + t('a template-literal title is a string literal too', + siteNames('site-template', { 'a.e2e.test.ts': suite(`it(\`boots\`, () => {\n${BULK}\n});`) }) + === names('it("boots")')); + t('...but a title with SUBSTITUTIONS is not, so the callee alone names it', + siteNames('site-template-sub', { 'a.e2e.test.ts': suite(`it(\`boots \${n}\`, () => {\n${BULK}\n});`) }) + === names('it')); + t('a title spanning lines is collapsed to one line, and never truncated', + siteNames('site-title-wrap', { 'a.e2e.test.ts': suite(`it(\`boots\nthe child\`, () => {\n${BULK}\n});`) }) + === names('it("boots the child")')); + + // The named limit, pinned rather than discovered: a property-access callee + // is deliberately NOT captured, because `promise.then(...)` and `rows.map( + // ...)` would be worse site names than the test block the walk reaches. + t('an it.skip() callee is a property access, not an identifier, so it still reads (top-level)', + siteNames('site-property-callee', { 'a.e2e.test.ts': suite(`it.skip('x', () => {\n${BULK}\n});`) }) + === names('(top-level)')); + + // ⭐ The collision END TO END, through the real classification path in + // audit(). One synthesised entry, two sibling blocks. Before this fix + // both rows keyed to `::(top-level)`, so ONE entry classified BOTH + // as deliberate and the second was never reviewed by the PR that needed + // it -- the carve-out-by-accident this file's header names. + const siblingRoot = tree('carve-siblings', { + 'a.e2e.test.ts': suite(`it('first', () => {\n${BULK}\n});\nit('second', () => {\n${BULK}\n});`), + }); + const fnsOf = (result) => JSON.stringify({ + deliberate: result.deliberate.map((row) => row.fn), + findings: result.findings.map((row) => row.fn), + }); + + const onlyFirst = audit(siblingRoot, { 'packages/cli/test/a.e2e.test.ts::it("first")': { why: 'synthetic' } }); + t('a registry entry for ONE it() block silences THAT block and leaves its sibling a finding', + fnsOf(onlyFirst) === JSON.stringify({ deliberate: ['it("first")'], findings: ['it("second")'] }), + fnsOf(onlyFirst)); + + const topLevelEntry = audit(siblingRoot, { 'packages/cli/test/a.e2e.test.ts::(top-level)': { why: 'synthetic' } }); + t('...and a (top-level) entry silences NEITHER -- before this fix that ONE entry silenced BOTH', + fnsOf(topLevelEntry) === JSON.stringify({ deliberate: [], findings: ['it("first")', 'it("second")'] }), + fnsOf(topLevelEntry)); + // -- (9) RULE 2 (#11595): a spawn CALL that leaves its env undeclared --- // The reds here are the whole point of the rule, so they are pinned by // REASON as well as by count: "reds for some reason" would still pass if @@ -1199,6 +1350,14 @@ export function selfTest() { t('a commented-out env-less spawn is not a call site', undeclared('envless-prose', { ...COMPANION, 'a.e2e.test.ts': spawner(' // execFile(\'x\', []);\n void cwd;') }).length === 0); + // Rule 2 has no registry, so it cannot MIS-KEY -- but it names sites in its + // messages, and `(top-level)` was as unhelpful there. Both rules read the + // same walk, so this is ONE change and not two (#12531). + t('rule 2 names the it() block too, so an undeclared env is not reported at (top-level)', + JSON.stringify(undeclared('envless-in-it', { + 'a.e2e.test.ts': suite('it(\'spawns a probe\', () => {\n execFile(\'x\', []);\n});'), + }).map((row) => row.fn)) === names('it("spawns a probe")')); + // -- (10) the ratchet, in every direction it must move ----------------- const one = [{ file: 'packages/cli/test/a.ts', fn: 'runCli', line: 1, text: 'x' }]; const two = [...one, { file: 'packages/cli/test/a.ts', fn: 'runOther', line: 2, text: 'x' }]; @@ -1361,7 +1520,8 @@ export function selfTest() { + '(a bare spread in a spawner reds OUT OF PROCESS and the childEnv() form exits zero through the same entry point; ' + 'an env-less spawn reds out of process too, by reason, through every spelling that reaches a spawn API; ' + 'a legitimate non-spawner bulk copy stays green and the same body reds once the file spawns; ' - + 'every member read stays green; the two rules red independently of each other; the carve-out is site-scoped; ' + + 'every member read stays green; the two rules red independently of each other; the carve-out is site-scoped, ' + + 'and a callback arrow is named after its CALL so two it() blocks in one file cannot share a registry key; ' + 'the ratchet fails in both directions; and all four refusals are paired with a tree that still returns a verdict).', ); return 0;