From 65e9a3fc0327fe205c61915e86dc0ecbc6a95f1f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 19 Sep 2026 11:40:26 +0000 Subject: [PATCH 1/2] fix(tooling): let the clocked-window rule see a load routed through a same-package helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The clocked-window rule in `check-test-source-alias` classifies loads by scanning comment-masked source for a LITERAL specifier sitting inside an `import(...)` / `require(...)` form. A wrapper offers no such site: the test writes `loadOptionalPackage('@objectstack/cloud-connection')` and the helper one frame down writes `await import(specifier)` — a VARIABLE. The file therefore earned no `clocked` entry AND no `moduleScope` entry, so both of the conditions the diagnostic needs were unmet at once and the gate stayed silent over a probe measured at 5005ms against a 5000ms `testTimeout`. The reader now follows ONE hop: a callee declared in the test file itself, or imported through a RELATIVE specifier resolving inside the same package, whose body performs a dynamic load of a non-literal specifier. Everything else is unchanged — the population is still the specifiers the package was already measured to resolve through `dist/`, read at the same granularity. A callee reached through a bare specifier is another package's code and is not followed; one that imports a literal is already read at its own site. Measured over the whole repo: the reader newly reports zero files, because the one call site it reaches (`packages/cli/src/utils/optional-package.test.ts`) already pays the load at module top. With that module-top import ablated in memory, the old reader reports nothing and the new one reports that call — the defect this fixes, reproduced on today's tree. The cheaper variant priced against it — flag a test file naming a registered specifier as a string literal anywhere with no module-scope load — reports 502 findings across 418 files and not one true positive: 383 type-only import clauses, 78 `vi.mock` registrations, 25 plain fixture strings and 16 specifiers quoted inside generated-source literals. Claude-Session: https://claude.ai/code/session_017ef78bLdybu3AffehKkhfk Co-authored-by: Claude --- scripts/check-test-source-alias.mjs | 335 +++++++++++++++++++++++++++- 1 file changed, 324 insertions(+), 11 deletions(-) diff --git a/scripts/check-test-source-alias.mjs b/scripts/check-test-source-alias.mjs index b9d44ad0436..680a0a39f43 100644 --- a/scripts/check-test-source-alias.mjs +++ b/scripts/check-test-source-alias.mjs @@ -274,6 +274,25 @@ // function — the silent direction, chosen because the loud one would fail // tests over a parse this file cannot afford to get right (see `asPath` for // why evaluating instead of reading was rejected). +// - **The load is read one hop through a same-package helper (#17658).** The +// scan above wants a LITERAL specifier sitting inside an import form, and a +// wrapper offers neither site: `loadOptionalPackage('@objectstack/cloud- +// connection')` hands the specifier to a helper that imports a VARIABLE, so +// the file got no `clocked` entry AND no `moduleScope` entry — both of the +// conditions the diagnostic needs, unmet at once. The result was silence, +// with CI green, over a probe measured at 5005ms against the default 5000ms +// `testTimeout` (#17180, on a built worktree). The hop is followed when the +// callee is declared in the test file itself, or imported through a RELATIVE +// specifier resolving INSIDE this package, and that callee's body performs a +// dynamic load of a NON-literal specifier. A callee reached through a bare +// specifier is another package's code and is not followed; one that imports +// a literal is already read at its own site, and following it would report +// the same load twice. +// ⛔ REACH, not population. The specifier reported is still one this package +// was already measured to resolve through `dist/` — the same set, read at the +// same granularity. A specifier whose ONLY mention anywhere in a package is +// such a call never enters that measurement and is still not reported; +// widening the population is a different card. // // ── The registry, and why it is shaped like this ──────────────────────────── // @@ -818,6 +837,18 @@ function maskedProjections(source) { return { commentsOnly: maskComments(source), codeOnly: maskCommentsAndLiterals(source) }; } +/** + * One file's projections and its function-body ranges, computed once. + * + * Both readers below want all three, and masking is the expensive half of this + * gate (see `importCache`), so a file is projected once and the context handed + * down rather than re-derived per rule. + */ +function sourceScanContext(source) { + const { commentsOnly, codeOnly } = maskedProjections(source); + return { commentsOnly, codeOnly, ranges: functionBodyRanges(codeOnly) }; +} + /** * The `<` matching a closing `>`, for a return-type annotation. -1 if none. * @@ -961,9 +992,8 @@ function functionBodyRanges(code) { * function body, with the line and the spelling the diagnostic quotes. Type-only * clauses and `typeof import(…)` queries appear in neither: they never resolve. */ -function moduleLoadSites(source) { - const { commentsOnly, codeOnly } = maskedProjections(source); - const ranges = functionBodyRanges(codeOnly); +function moduleLoadSites(source, ctx = sourceScanContext(source)) { + const { commentsOnly, codeOnly, ranges } = ctx; const inFunction = (index) => ranges.some(([start, end]) => index > start && index < end); const moduleScope = new Set(); const clocked = []; @@ -998,23 +1028,180 @@ function lineOf(source, index) { return line; } +// ── the one hop: a load routed through a same-package helper (#17658) ───── + +/** + * A dynamic load whose specifier is NOT a literal — `await import(specifier)`. + * + * This is what makes a callee a loader rather than an ordinary function: the + * specifier it loads is whatever it was handed, so the literal lives at the CALL + * SITE and nowhere else. A helper importing a literal is deliberately excluded — + * the scanner above already reads that site, and reading it twice would report + * one load as two. + */ +const VARIABLE_LOAD = /\b(?:import|require)\s*\(\s*[A-Za-z_$]/; + +/** + * A call whose first argument is a quoted string, with the callee captured. + * The cheap pre-filter and the site reader both run this; `lastIndex` is reset + * at every entry because it is shared. + */ +const CALL_WITH_LITERAL = /\b([A-Za-z_$][\w$]*)\s*\(\s*['"]([^'"]+)['"]/g; + +/** + * A relative `import … from './x.js'` — the only hop this rule follows. The + * clause class is `IMPORT_PATTERNS`' own, bounded to one statement for the + * reason #12555 records. + */ +const RELATIVE_IMPORT_CLAUSE = /\bimport\s+([^;'"]*?)\s*from\s*['"](\.[^'"]*)['"]/g; + +/** + * Names this source declares whose function body performs a `VARIABLE_LOAD`. + * + * Both spellings a helper is written in are read, and the value form must BE a + * function (`= (`, `= function`, `= x =>`) so that a plain `const` cannot adopt + * the body of whatever function happens to follow it. A declaration owns the + * first body range that opens after it and before the NEXT declaration, which is + * what keeps a multi-line signature — `}): Promise< { … } > {`, the shape that + * defeated the brace scanner once already — attached to its own function. + */ +const HELPER_DECLARATION = + /\b(?:export\s+)?(?:async\s+)?function(?:\s*\*)?\s+([A-Za-z_$][\w$]*)|\b(?:export\s+)?(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:async\s+)?(?:\(|function\b|[A-Za-z_$][\w$]*\s*=>)/g; + +function dynamicVariableLoaders(ctx) { + if (!VARIABLE_LOAD.test(ctx.codeOnly)) return new Set(); + const ranges = [...ctx.ranges].sort((a, b) => a[0] - b[0]); + const decls = []; + HELPER_DECLARATION.lastIndex = 0; + let match; + while ((match = HELPER_DECLARATION.exec(ctx.codeOnly))) decls.push({ name: match[1] ?? match[2], at: match.index }); + const names = new Set(); + for (let i = 0; i < decls.length; i++) { + const next = decls[i + 1]?.at ?? ctx.codeOnly.length; + const body = ranges.find(([start]) => start > decls[i].at && start < next); + if (!body) continue; + if (VARIABLE_LOAD.test(ctx.codeOnly.slice(body[0], body[1]))) names.add(decls[i].name); + } + return names; +} + +/** `path -> loader names`, for the whole process: a helper serves many tests. */ +const helperLoaderCache = new Map(); + +function helperLoadersOf(helperFile) { + const cached = helperLoaderCache.get(helperFile); + if (cached) return cached; + let names; + try { + names = dynamicVariableLoaders(sourceScanContext(readFileSync(helperFile, 'utf8'))); + } catch { + names = new Set(); + } + helperLoaderCache.set(helperFile, names); + return names; +} + +/** Is `candidate` inside `dir`? `relative()` climbing out is the whole test. */ +function isInsidePackage(dir, candidate) { + const rel = relative(dir, candidate); + return rel.length > 0 && !rel.startsWith('..'); +} + /** - * The clocked-window findings for one test file: a dynamic load of a specifier - * this package resolves through `dist/`, inside a function body, with no + * `local name -> how the hop is spelled`, for every binding in this test file + * that routes a specifier into a dynamic load one frame down. + * + * Two sources, and no third: a loader declared in this very file, and one + * imported through a relative specifier that resolves INSIDE this package. A + * bare specifier is another package's code — following it would make this rule + * a cross-package walk, which is a different reader and a different card. + */ +function helperLoaderBindings(file, ctx, packageDir) { + const bindings = new Map(); + for (const name of dynamicVariableLoaders(ctx)) bindings.set(name, 'declared in this file'); + RELATIVE_IMPORT_CLAUSE.lastIndex = 0; + let match; + while ((match = RELATIVE_IMPORT_CLAUSE.exec(ctx.commentsOnly))) { + const clause = match[1]; + if (isTypeOnlyClause(clause)) continue; + const target = resolveRelative(file, match[2]); + if (!target || !isInsidePackage(packageDir, target)) continue; + const loaders = helperLoadersOf(target); + if (loaders.size === 0) continue; + const braced = clause.trim().match(/\{([\s\S]*)\}/); + if (!braced) continue; + for (const piece of braced[1].split(',')) { + const spelt = piece.trim(); + if (!spelt || /^type\s/.test(spelt)) continue; + const renamed = spelt.match(/^([A-Za-z_$][\w$]*)\s+as\s+([A-Za-z_$][\w$]*)$/); + const exported = renamed ? renamed[1] : spelt; + const local = renamed ? renamed[2] : spelt; + if (loaders.has(exported)) bindings.set(local, `via \`${match[2]}\``); + } + } + return bindings; +} + +/** + * The clocked loads this file pays through a helper: a call, inside a function + * body, handing a literal specifier to a binding that loads a variable. + * + * Shaped like a `clocked` entry so the diagnostic needs no second spelling — + * `form` is the callee, so the message quotes the call the author actually + * wrote, and `via` names the hop. + */ +function helperRoutedLoadSites(file, ctx, offendingSpecs, packageDir) { + const bindings = helperLoaderBindings(file, ctx, packageDir); + if (bindings.size === 0) return []; + const sites = []; + CALL_WITH_LITERAL.lastIndex = 0; + let match; + while ((match = CALL_WITH_LITERAL.exec(ctx.commentsOnly))) { + const [, callee, spec] = match; + if (!bindings.has(callee)) continue; + if (!offendingSpecs.has(spec)) continue; + if (!ctx.ranges.some(([start, end]) => match.index > start && match.index < end)) continue; + sites.push({ spec, form: callee, line: lineOf(ctx.source, match.index), via: bindings.get(callee) }); + } + return sites; +} + +/** Does any call in this file hand a literal from the measured set to anything? */ +function mightRouteALoad(source, offendingSpecs) { + CALL_WITH_LITERAL.lastIndex = 0; + let match; + while ((match = CALL_WITH_LITERAL.exec(source))) if (offendingSpecs.has(match[2])) return true; + return false; +} + +/** + * The clocked-window findings for one test file: a load of a specifier this + * package resolves through `dist/`, paid inside a function body, with no * module-scope load of the same specifier anywhere in the file. + * + * Two readers, one population. The first wants a literal specifier inside an + * import form; the second follows one hop into a same-package helper that + * imports a variable (#17658). Findings are merged in source order and deduped + * per specifier, so a file that pays the same load both ways reports once. */ -function clockedWindowFindings(file, offendingSpecs) { +function clockedWindowFindings(file, offendingSpecs, packageDir) { let source; try { source = readFileSync(file, 'utf8'); } catch { return []; } - if (!/\b(?:import|require)\s*\(/.test(source)) return []; - const { moduleScope, clocked } = moduleLoadSites(source); + const direct = /\b(?:import|require)\s*\(/.test(source); + const routed = mightRouteALoad(source, offendingSpecs); + if (!direct && !routed) return []; + const ctx = { source, ...sourceScanContext(source) }; + const { moduleScope, clocked } = moduleLoadSites(source, ctx); + const sites = routed + ? [...clocked, ...helperRoutedLoadSites(file, ctx, offendingSpecs, packageDir)].sort((a, b) => a.line - b.line) + : clocked; const seen = new Set(); const findings = []; - for (const site of clocked) { + for (const site of sites) { if (!offendingSpecs.has(site.spec)) continue; if (moduleScope.has(site.spec)) continue; if (seen.has(site.spec)) continue; @@ -1689,7 +1876,7 @@ function scan(root) { const clockedLoads = []; if (offendingSpecs.size > 0) { for (const file of reachable.testFiles.sort()) { - for (const finding of clockedWindowFindings(file, offendingSpecs)) { + for (const finding of clockedWindowFindings(file, offendingSpecs, pkg.dir)) { clockedLoads.push({ ...finding, file: relative(root, file) }); } } @@ -1830,6 +2017,12 @@ function check(root, registry) { for (const load of pkg.clockedLoads ?? []) { failures.push( `${load.file}:${load.line}: \`${load.form}('${load.spec}')\` is paid inside a function body — a CLOCKED window.\n` + + (load.via + ? ` The literal specifier is the ARGUMENT: \`${load.form}\` (${load.via}) hands it to a dynamic\n` + + ' `import()` of a VARIABLE one frame down, so the specifier is literal HERE and nowhere else.\n' + + ' The transform is identical either way — the indirection changes who writes the specifier,\n' + + ' not what gets loaded or when.\n' + : '') + ` This file has no module-scope load of \`${load.spec}\`, and this package resolves that specifier\n` + ' through `dist/`, so the first call transforms that dependency\'s whole module graph while a\n' + " `testTimeout` or `hookTimeout` is running. Measured on the incident this rule comes from: 3.1-3.6s\n" + @@ -2435,6 +2628,71 @@ function buildFixtureTree() { '});\n', }); + // ── (27-28) THE ONE HOP (#17658) ──────────────────────────────── + // + // The shape the literal scanner cannot see: the specifier is a literal, but it + // sits in ARGUMENT position, and the `import()` one frame down takes a + // VARIABLE. Measured on the real instance (`packages/cli/src/utils/ + // optional-package.test.ts`, #17180): the file got no `clocked` entry and no + // `moduleScope` entry, so both of the diagnostic's conditions were unmet at + // once and the gate said nothing while the probe took 5005ms against a 5000ms + // `testTimeout`. + // + // A helper living in ANOTHER package, exporting an identically-shaped loader, + // so the "one hop, SAME package" bound is pinned by a case that would really + // be reported if the hop were followed through a bare specifier. + fixture(root, 'packages/remote-loader', { + 'package.json': ARTIFACT_MANIFEST('@fx/remote-loader'), + 'src/index.ts': + 'export async function loadThing(specifier: string): Promise {\n' + + ' return await import(specifier);\n' + + '}\n', + }); + + // (27) VIOLATING, routed. `ledger.test.ts` is what puts the three specifiers + // in this package's measured set — exactly how `@objectstack/cloud-connection` + // is in `@objectstack/cli`'s while the routed file pays no module-top load of + // it. It is a separate file on purpose: a module-top load in THIS file would + // pay the transform during collection and the finding would be wrong. + fixture(root, 'packages/clocked-helper-routed', { + 'package.json': ARTIFACT_MANIFEST('@fx/clocked-helper-routed'), + 'src/loader.ts': + 'export async function loadThing(specifier: string): Promise {\n' + + ' return await import(specifier);\n' + + '}\n' + + 'export function describeThing(name: string): string {\n' + + ' return name;\n' + + '}\n', + 'src/ledger.test.ts': "import '@fx/core';\nimport '@fx/core/logger';\nimport '@fx/core/nested';\nexport default 1;\n", + 'src/thing.test.ts': + "import { it } from 'vitest';\n" + + "import { loadThing, describeThing } from './loader.js';\n" + + "import { loadThing as foreignLoad } from '@fx/remote-loader';\n" + + "it('y', async () => {\n" + + " const mod = await loadThing('@fx/core');\n" + + " const label = describeThing('@fx/core/logger');\n" + + " const other = await foreignLoad('@fx/core/nested');\n" + + ' return [mod, label, other];\n' + + '});\n', + }); + + // (28) THE CONTROL. The same routed call, in a file that already pays the + // load at module top — the remedy this gate prints, and the state the real + // instance is in today. Reporting it would mean the reader had stopped asking + // WHERE the load is paid and started reporting the indirection itself. + fixture(root, 'packages/clocked-helper-routed-paid', { + 'package.json': ARTIFACT_MANIFEST('@fx/clocked-helper-routed-paid'), + 'src/loader.ts': + 'export async function loadThing(specifier: string): Promise {\n' + + ' return await import(specifier);\n' + + '}\n', + 'src/thing.test.ts': + "import { it } from 'vitest';\n" + + "import '@fx/core';\n" + + "import { loadThing } from './loader.js';\n" + + "it('y', async () => loadThing('@fx/core'));\n", + }); + return root; } @@ -2466,7 +2724,7 @@ const SELF_TEST_BATTERIES = Object.freeze({ 'the canary (#8020)': 14, 'the cross-boundary walk (#8351)': 7, 'the latent half (#9674)': 8, - 'the clocked-window rule (#10126)': 12, + 'the clocked-window rule (#10126)': 19, 'the import clause is bounded to ONE statement (#12555)': 11, 'the declared population must stay READABLE by the dispatch deriver': 11, 'the declaration must still BE the workspace (#11510)': 22, @@ -2857,6 +3115,61 @@ function selfTest() { 'the clocked-window finding for the literal-brace fixture lost or renamed its specifier', ); + // ── THE ONE HOP (#17658) ────────────────────────────────────────────── + // + // Needles carry the FULL path for the reason the `clocked-load` pair + // records: `packages/clocked-helper-routed` is a prefix of + // `packages/clocked-helper-routed-paid`, so a loose needle would read the + // control fixture's silence as this one's. + expect( + clockedIn('packages/clocked-helper-routed/src/thing.test.ts:5').some((f) => + f.includes("loadThing('@fx/core')"), + ), + 'a load routed one hop through a same-package helper went unseen — the literal scanner is still the only reader (#17658)', + ); + // The diagnostic has to say WHY this call is a load at all, or the author + // reads it as a false positive: the callee is named, the hop is named, and + // the reason the specifier is literal only here is stated. + expect( + clockedIn('packages/clocked-helper-routed/src/thing.test.ts:5').every( + (f) => f.includes('via `./loader.js`') && f.includes('VARIABLE'), + ), + 'the routed finding did not name the hop it followed — an author cannot tell it from a false positive', + ); + // …and it still prints the one remedy, which is unchanged by the routing. + expect( + clockedIn('packages/clocked-helper-routed/src/thing.test.ts:5').every( + (f) => f.includes('COLLECTION') && f.includes("import '@fx/core';"), + ), + 'the routed finding printed no module-top import to add — the remedy is the same one, not a new one', + ); + // BOUND ONE: the callee must be a LOADER. `describeThing` is exported from + // the same helper module and takes the same kind of argument, so a reader + // that follows the module rather than the function reports this too. + expect( + clockedIn('packages/clocked-helper-routed/src/thing.test.ts').every((f) => !f.includes('@fx/core/logger')), + 'a call to a non-loading export of the same helper module was reported — the hop follows the MODULE, not the function', + ); + // BOUND TWO: ONE hop, SAME package. `@fx/remote-loader` exports a loader of + // exactly the shape followed above, reached through a BARE specifier — so + // this leg fails loudly if the rule ever becomes a cross-package walk. + expect( + clockedIn('packages/clocked-helper-routed/src/thing.test.ts').every((f) => !f.includes('@fx/core/nested')), + 'a helper reached through a BARE specifier was followed — the one-hop rule has become a cross-package walk', + ); + // One file, one finding: the two bounds above are the whole difference. + expect( + clockedIn('packages/clocked-helper-routed/src/thing.test.ts').length === 1, + 'the one-hop reader did not report exactly the one routed load this fixture pays in a clocked window', + ); + // THE CONTROL: the same routed call with the module top already paying it. + // This is the state the real #17180 instance is in today, so a reader that + // reports it has turned the remedy into the defect. + expect( + clockedIn('packages/clocked-helper-routed-paid/src/thing.test.ts').length === 0, + 'a routed call in a file that already loads the specifier at module top was reported — that is the remedy, not the defect', + ); + // ── the import clause is bounded to ONE statement (#12555) ──────────── // // All three rows of the card's table, pinned as a set comparison so the two From a66ebc98141f1aa524c489acd19ce03fb3be1644 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 19 Sep 2026 11:42:23 +0000 Subject: [PATCH 2/2] test(tooling): make the two positive one-hop assertions fail under ablation An `every()` over an empty finding list is vacuously true, so both text assertions passed with the reader disabled and only the presence and the count legs drove red. Pairing each with the count makes four of the seven new cases fail when the reader is removed. Claude-Session: https://claude.ai/code/session_017ef78bLdybu3AffehKkhfk Co-authored-by: Claude --- scripts/check-test-source-alias.mjs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/scripts/check-test-source-alias.mjs b/scripts/check-test-source-alias.mjs index 680a0a39f43..34ee8f00135 100644 --- a/scripts/check-test-source-alias.mjs +++ b/scripts/check-test-source-alias.mjs @@ -3131,16 +3131,18 @@ function selfTest() { // reads it as a false positive: the callee is named, the hop is named, and // the reason the specifier is literal only here is stated. expect( - clockedIn('packages/clocked-helper-routed/src/thing.test.ts:5').every( - (f) => f.includes('via `./loader.js`') && f.includes('VARIABLE'), - ), + clockedIn('packages/clocked-helper-routed/src/thing.test.ts:5').length === 1 && + clockedIn('packages/clocked-helper-routed/src/thing.test.ts:5').every( + (f) => f.includes('via `./loader.js`') && f.includes('VARIABLE'), + ), 'the routed finding did not name the hop it followed — an author cannot tell it from a false positive', ); // …and it still prints the one remedy, which is unchanged by the routing. expect( - clockedIn('packages/clocked-helper-routed/src/thing.test.ts:5').every( - (f) => f.includes('COLLECTION') && f.includes("import '@fx/core';"), - ), + clockedIn('packages/clocked-helper-routed/src/thing.test.ts:5').length === 1 && + clockedIn('packages/clocked-helper-routed/src/thing.test.ts:5').every( + (f) => f.includes('COLLECTION') && f.includes("import '@fx/core';"), + ), 'the routed finding printed no module-top import to add — the remedy is the same one, not a new one', ); // BOUND ONE: the callee must be a LOADER. `describeThing` is exported from