From 17c25687d0cf0d86cd70f675b8a2df35d625e6d3 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Mon, 10 Aug 2026 15:25:10 +0200 Subject: [PATCH 1/3] fix(#642): fail closed on computed dynamic imports in check-boundaries RULES check-boundaries.mjs's generic RULES loop and Rule B relied on a hand-rolled dynamic-import regex (extractSpecifiers's fourth pattern) that could only extract an argument already shaped like a complete literal. A computed dynamic import either matched nothing at all (silently exempting it from every layering rule) or, on a concatenated expression like import('../' + name), risked being partially matched into just its quoted prefix -- neither is acceptable for a boundary check, where inability to prove a target statically is itself the violation. Added two shared, parser-backed helpers to build/lib/check-legacy-owners.mjs: findDynamicImportUsages (a real TypeScript-parser walk classifying every dynamic import(...) call as { kind: 'static', spec } for a string/ no-substitution-template-literal argument, or { kind: 'uncheckable' } for everything else -- identifier, computed template, concatenation, conditional, or any other shape, never silently dropped) and mightContainDynamicImport (a conservative textual gate that only asks whether an `import` keyword could be followed by legal trivia and `(`, deliberately preferring false positives, never inspecting the argument). check-boundaries.mjs now runs a fail-closed pre-pass over the deduplicated set of files covered by any generic RULES entry, once per unique file (so nested rule directories like src/dashboard/src/dashboard/application never double-report the same occurrence): every uncheckable dynamic import is an unconditional violation, and every static one flows through the existing relative-resolution/forbidden-prefix logic exactly like an ordinary static import. Rule B reuses the same cached classification instead of its own dynamic regex. Rule C/Rule D and the other already-parser-backed package guards are untouched. extractSpecifiers is renamed extractStaticSpecifiers to make its narrowed static-only role explicit. Added tests/unit/check-boundaries-dynamic-imports.test.js for the shared classifier/gate in isolation, and extended dashboard-boundaries.test.js and clickhouse-http-package-policy.test.js's Rule A/B policy mirrors with the full fail-closed sabotage set (single/double/no-substitution-template static forms, computed template, identifier, concatenation, conditional) plus production drift binds proving the real checker calls the new shared helpers and no longer carries the old dynamic-import regex arm. Audited every other regex/prefilter in check-boundaries.mjs per the repo's architecture-guard footgun: mightReferencePackage, mightReferenceForbiddenRelativeDir, and mightReferenceRetiredTopLevelApi are all unchanged and still scoped to their original threat models; every resolved-path comparison still canonicalizes through resolveRelative's fs.realpathSync. check:arch timing is unchanged (~15.3-15.8s before and after across 3 runs each) -- no caching/gating changes were needed. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- CHANGELOG.md | 19 ++ build/check-boundaries.mjs | 169 +++++++++++---- build/lib/check-legacy-owners.mjs | 116 ++++++++++ .../check-boundaries-dynamic-imports.test.js | 171 +++++++++++++++ .../clickhouse-http-package-policy.test.js | 202 ++++++++++++++++-- tests/unit/dashboard-boundaries.test.js | 135 ++++++++++++ 6 files changed, 755 insertions(+), 57 deletions(-) create mode 100644 tests/unit/check-boundaries-dynamic-imports.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index a0ae7ed0..800f1a48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -448,6 +448,25 @@ auto-generated per-PR notes; this file is the curated, human-readable history. throughout: `src/net/ch-client.ts` remains authoritative; no `src/**` code changed at any point across either amendment. +### Fixed +- **#642: `check:arch`'s generic layering rules (and Rule B) now fail closed + on a computed dynamic `import(...)` instead of silently skipping it.** + `extractSpecifiers` (renamed `extractStaticSpecifiers`) used to include a + dynamic-import regex arm that could only ever extract an argument that + already looked like a complete literal — a computed expression such as + `import('../' + name)` either matched nothing (exempting it from every + `RULES` boundary entirely) or risked being partially matched into just its + quoted prefix. Every dynamic import under a guarded directory is now + classified by a new shared, parser-backed helper + (`findDynamicImportUsages`/`mightContainDynamicImport`, + `build/lib/check-legacy-owners.mjs`): a single-quoted, double-quoted, or + no-substitution-template-literal argument remains statically analyzable and + flows through the same relative-resolution/forbidden-prefix logic as an + ordinary static import; every other argument shape (identifier, computed + template, concatenation, conditional, or otherwise) is an unconditional + violation. No product/runtime behavior changed — this hardens the + architecture gate's own soundness, not the policy it enforces. + ## [0.7.3] - 2026-08-06 ### Added diff --git a/build/check-boundaries.mjs b/build/check-boundaries.mjs index a0207d6f..bde2beaf 100644 --- a/build/check-boundaries.mjs +++ b/build/check-boundaries.mjs @@ -14,21 +14,21 @@ // second script. // // Hand-rolled regex scan for the internal src-layering rules (RULES below) -// and Rule B's zero-bare-specifier check: the codebase has no exotic import -// syntax there, so scanning for import/export specifiers is enough and keeps -// those rules a zero-dependency, sub-second pretest step. The exceptions are -// the former-owner rules, Rule C (the package relative-deep-import ban, -// Guard 2 — issue #630 Phase 8, review pass 1), BOTH halves of the revised -// package Rule D (the deep-import-subpath ban and the bare-specifier -// name/shape check), and the `@clickhouse/client-web` reintroduction ban -// (Guard 5) below — all of which need identifier/import-shape-level (not -// specifier-text-level) detection and therefore delegate to a real -// TypeScript parse in `build/lib/check-legacy-owners.mjs` — see that module -// for why textual matching was retired there (issue #630 Phase 3), and why -// the same real-parser mechanism (not a new hand-rolled scanner) was -// required again for issue #630 Phase 5's revised Rule D, and again for -// issue #630 Phase 8's Rule C/Guard 2 broadening and Guard 5: a comment -// sitting between `import`/`export` and the specifier, or an escaped +// and Rule B's zero-bare-specifier check: the codebase has no exotic STATIC +// import syntax there, so scanning for static import/export specifiers is +// enough and keeps those rules a zero-dependency, sub-second pretest step for +// their static forms. The exceptions are the former-owner rules, Rule C (the +// package relative-deep-import ban, Guard 2 — issue #630 Phase 8, review pass +// 1), BOTH halves of the revised package Rule D (the deep-import-subpath ban +// and the bare-specifier name/shape check), and the `@clickhouse/client-web` +// reintroduction ban (Guard 5) below — all of which need identifier/ +// import-shape-level (not specifier-text-level) detection and therefore +// delegate to a real TypeScript parse in `build/lib/check-legacy-owners.mjs` +// — see that module for why textual matching was retired there (issue #630 +// Phase 3), and why the same real-parser mechanism (not a new hand-rolled +// scanner) was required again for issue #630 Phase 5's revised Rule D, and +// again for issue #630 Phase 8's Rule C/Guard 2 broadening and Guard 5: a +// comment sitting between `import`/`export` and the specifier, or an escaped // string-literal segment, defeats a regex (however far its // whitespace/delimiter patterns are widened) but is ordinary parser // trivia/decoded text to a real parse — review pass 1 confirmed Rule C's @@ -36,6 +36,35 @@ // this file's own stated Phase 8 design goal, while its unit-test mirror // independently reimplemented the identical regex rather than calling the // real parser. +// +// Issue #642 — dynamic `import(...)` calls under the generic `RULES` loop +// (and Rule B) are a FOURTH exception, on top of the three above, for a +// different reason: the former dynamic-import arm of the regex below +// (`extractSpecifiers`, now renamed `extractStaticSpecifiers` to make its +// narrowed role explicit) could only ever extract a specifier that LOOKED +// like a complete literal — a computed expression such as +// `import('../' + name)` either matched nothing (silently exempting it from +// every rule below) or, worse, could be partially matched into just its +// quoted prefix. Neither is acceptable for a boundary check: an import whose +// target cannot be statically proven must fail, not fall through as if it +// were absent. Every generic-guarded file is now additionally classified by +// the shared real-parser helpers `findDynamicImportUsages`/ +// `mightContainDynamicImport` below — `mightContainDynamicImport` gates the +// cheap case (a file that provably contains no `import(...)` call skips the +// parser entirely, preserving the ordinary static fast path for files with no +// dynamic import at all); `findDynamicImportUsages` then classifies every +// dynamic-import call expression in a matched file as `{ kind: 'static', +// spec }` (a plain string/no-substitution-template-literal argument, fed +// through the exact same relative-resolution/forbidden-prefix logic as an +// ordinary static import) or `{ kind: 'uncheckable' }` (everything else — +// identifier, computed template, concatenation, conditional, or any other +// expression shape — an UNCONDITIONAL violation, independent of which rule +// eventually would have matched the file). Computed/non-static dynamic +// imports are forbidden in every source file covered by at least one generic +// `RULES` entry; Rule C/Rule D and the other already-parser-backed package +// guards are untouched by this — they already classify their own dynamic +// imports through the real parser and #630/#646/#653's package-guard work is +// not being redone here. import fs from 'node:fs'; import path from 'node:path'; @@ -64,6 +93,8 @@ import { manifestDependencyFields, lockHasPackage, retiredClientSpikeScriptNames, + findDynamicImportUsages, + mightContainDynamicImport, } from './lib/check-legacy-owners.mjs'; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); @@ -221,31 +252,35 @@ function collectFiles(target) { } // Matches, in order: static `import ... from '...'` (incl. `import type`), -// `export ... from '...'` (incl. `export type`), a bare side-effect -// `import '...'`, and dynamic `import('...')`. Each pattern requires only -// identifier/brace/comma/whitespace characters between the keyword and -// `from`, so it can't skip past a from-less import into a later statement's -// clause, and `\b` keeps it off the word "import" inside an identifier. -// Used only by the checks named in the comment above (internal src layering, -// the @clickhouse/client-web ban, Rule B) — NEITHER half of Rule D's -// `@altinity/clickhouse-http` check calls this anymore (both now delegate to -// the real-parser helpers in `build/lib/check-legacy-owners.mjs`, below). +// `export ... from '...'` (incl. `export type`), and a bare side-effect +// `import '...'`. Each pattern requires only identifier/brace/comma/ +// whitespace characters between the keyword and `from`, so it can't skip +// past a from-less import into a later statement's clause, and `\b` keeps it +// off the word "import" inside an identifier. Used only by the checks named +// in the comment above (internal src layering, the @clickhouse/client-web +// ban, Rule B) — NEITHER half of Rule D's `@altinity/clickhouse-http` check +// calls this anymore (both now delegate to the real-parser helpers in +// `build/lib/check-legacy-owners.mjs`, below). // -// Only the dynamic-import pattern also accepts a backtick-delimited -// no-substitution template literal (`` import(`pkg`) ``): a static -// import/export declaration's module specifier and a bare side-effect -// import's specifier must be a plain string literal per grammar — only a -// dynamic `import(...)` call can take a template literal argument — so -// widening the other three patterns to backticks would only ever match -// syntax that can't occur. +// Issue #642 — the FOURTH pattern this array used to carry (a dynamic +// `import(...)` call) is gone: it could only ever extract a specifier that +// LOOKED like a complete literal, so a computed dynamic import either +// matched nothing (silently exempting it from every rule below) or, on a +// concatenated expression like `import('../' + name)`, could be partially +// matched into just its quoted prefix — the exact "reduced to the quoted +// prefix" bug this issue closes. `extractStaticSpecifiers` (renamed from +// `extractSpecifiers` to make its narrowed role explicit) now handles ONLY +// the three ordinary static forms above; every dynamic `import(...)` call in +// a generic-guarded file is classified separately, by the real-parser helpers +// `findDynamicImportUsages`/`mightContainDynamicImport`, in the fail-closed +// pre-pass right before the `RULES` loop below. const SPECIFIER_PATTERNS = [ /\bimport\s+[\w*{}\s,]+\s+from\s*['"]([^'"]+)['"]/g, /\bexport\s+[\w*{}\s,]+\s+from\s*['"]([^'"]+)['"]/g, /\bimport\s*['"]([^'"]+)['"]/g, - /\bimport\s*\(\s*[`'"]([^`'"]+)[`'"]/g, ]; -function extractSpecifiers(source) { +function extractStaticSpecifiers(source) { const specs = []; for (const pattern of SPECIFIER_PATTERNS) { pattern.lastIndex = 0; @@ -293,15 +328,61 @@ function resolveRelative(fromFile, spec) { const violations = []; let checkedFiles = 0; let activeRules = 0; + +// Issue #642 — collect the FULL set of files covered by at least one generic +// RULES entry, deduplicated by absolute path, before evaluating any single +// rule. This matters because rule directories can nest (e.g. `src/dashboard` +// and `src/dashboard/application`: `collectFiles` on the outer dir already +// walks the inner one), so a naive per-rule loop would otherwise hand the +// same file's dynamic imports to the real parser once per matching rule and +// — if not careful — report the same uncheckable occurrence more than once. +// Each file's source is read exactly once here and reused by every rule below +// instead of re-reading it per rule. +const guardedFileSources = new Map(); // absolute path -> source text +const ruleFileLists = []; // { rule, files: absolute path[] } for (const rule of RULES) { const ruleDir = path.join(repoRoot, rule.dir); const files = fs.existsSync(ruleDir) ? collectFiles(ruleDir) : []; + ruleFileLists.push({ rule, files }); + for (const file of files) { + if (!guardedFileSources.has(file)) guardedFileSources.set(file, fs.readFileSync(file, 'utf8')); + } +} + +// Issue #642 — fail-closed dynamic-import pre-pass, run ONCE per unique +// guarded file (see the dedup rationale above), strictly before any +// individual RULES entry is evaluated: every `uncheckable` dynamic import +// (an identifier, a computed template, a concatenation, a conditional, or any +// other non-literal argument shape) is an unconditional violation, regardless +// of which rule(s) would otherwise have matched the file and regardless of +// where the import might have actually resolved — inability to prove that +// statically is itself the violation. `mightContainDynamicImport` gates the +// expensive real-parser call so a file that provably has no `import(...)` +// call anywhere never pays for it; a `static` result (a plain string or +// no-substitution-template-literal argument) is cached here and consumed by +// the RULES loop below exactly like an ordinary static import. +const guardedFileDynamicImports = new Map(); // absolute path -> DynamicImportUsage[] +for (const [file, source] of guardedFileSources) { + if (!mightContainDynamicImport(source)) continue; + const relFile = path.relative(repoRoot, file).split(path.sep).join('/'); + const usages = findDynamicImportUsages(source, relFile); + guardedFileDynamicImports.set(file, usages); + for (const usage of usages) { + if (usage.kind !== 'uncheckable') continue; + violations.push(`${relFile} → dynamic import(...) cannot be statically checked against the architecture boundary (issue #642: only a single-quoted, double-quoted, or no-substitution-template-literal specifier is statically analyzable)`); + } +} + +for (const { rule, files } of ruleFileLists) { if (files.length === 0) continue; // directory not born yet — rule activates with it activeRules += 1; checkedFiles += files.length; for (const file of files) { - const source = fs.readFileSync(file, 'utf8'); - for (const spec of extractSpecifiers(source)) { + const source = guardedFileSources.get(file); + const dynamicStaticSpecs = (guardedFileDynamicImports.get(file) ?? []) + .filter((usage) => usage.kind === 'static') + .map((usage) => usage.spec); + for (const spec of [...extractStaticSpecifiers(source), ...dynamicStaticSpecs]) { if (!spec.startsWith('.')) continue; // bare/package specifiers can't reach src dirs const resolved = resolveRelative(file, spec); const relResolved = path.relative(repoRoot, resolved).split(path.sep).join('/'); @@ -460,13 +541,27 @@ if (fs.existsSync(lockPath)) { // with '.', so a literal absolute-looking path would otherwise slip past // Rule A undetected — everything that isn't a relative specifier is a // violation here, with no exceptions. +// +// Issue #642 — this block used to independently re-run the (now-removed) +// dynamic-import arm of `extractSpecifiers`. It no longer needs any dynamic- +// import handling of its own: `packages/clickhouse-http/src` is ALSO a +// generic RULES entry (Rule A, above), so a COMPUTED dynamic import here +// already failed the fail-closed pre-pass before this block ever runs. What +// remains for Rule B is exactly the bare-vs-relative policy decision for a +// dynamic import whose specifier IS statically known — reusing the same +// cached source and cached `{ kind: 'static', spec }` results Rule A already +// produced, rather than re-reading the file or re-deriving the classification +// a second time. const PACKAGE_SRC_DIR = path.join(repoRoot, 'packages/clickhouse-http/src'); if (fs.existsSync(PACKAGE_SRC_DIR)) { for (const file of collectFiles(PACKAGE_SRC_DIR)) { const relFile = path.relative(repoRoot, file).split(path.sep).join('/'); checkedFiles += 1; - const source = fs.readFileSync(file, 'utf8'); - for (const spec of extractSpecifiers(source)) { + const source = guardedFileSources.get(file) ?? fs.readFileSync(file, 'utf8'); + const dynamicStaticSpecs = (guardedFileDynamicImports.get(file) ?? []) + .filter((usage) => usage.kind === 'static') + .map((usage) => usage.spec); + for (const spec of [...extractStaticSpecifiers(source), ...dynamicStaticSpecs]) { if (spec.startsWith('.')) continue; // relative — governed by Rule A above violations.push(`${relFile} → ${spec} (issue #630 Phase 2: clickhouse-http has zero bare package imports)`); } diff --git a/build/lib/check-legacy-owners.mjs b/build/lib/check-legacy-owners.mjs index 73bc5315..2597d84c 100644 --- a/build/lib/check-legacy-owners.mjs +++ b/build/lib/check-legacy-owners.mjs @@ -446,6 +446,122 @@ export function findModuleSpecifiers(source, filename) { }); } +// ── Issue #642 — fail-closed dynamic-import classification ────────────────── +// +// `findModuleSpecifiers` above (and `findDeepImportSpecifiers`/ +// `findPackageImportUsages` before it) all share the same accepted +// convention: a dynamic `import(...)` call whose first argument is not a +// plain string/no-substitution-template literal contributes NOTHING to their +// result — `specText`/`deepSpecifierText`/`isTargetSpecifier` all return +// `null`/`false` for that shape, and the caller's `if (spec !== null) +// push(...)` guard then silently drops it. That is the correct contract for +// those checks (a computed dynamic import naming a package/deep-subpath +// cannot be proven to reach that package, so it cannot be proven to violate +// THEIR rule either) but it is exactly the wrong contract for the generic +// `RULES` boundary in `build/check-boundaries.mjs`: that rule's whole point +// is that an import whose target cannot be statically determined must fail, +// not fall through as if it were absent. `findDynamicImportUsages` is a +// SEPARATE entry point (never a modification of the four existing dynamic- +// import branches above) that reports a discriminated union for every +// dynamic-import call expression, so nothing is ever silently dropped: a +// `{ kind: 'static', spec }` where the caller's existing resolution logic can +// treat `spec` exactly like an ordinary import, and a `{ kind: 'uncheckable' +// }` that must always become a violation in a generic-guarded file, +// regardless of what its argument might eventually resolve to. A concatenated +// expression such as `import('../' + name)` is `uncheckable` in full — never +// reduced to the quoted `'../'` prefix, since that half alone proves nothing +// about the complete runtime specifier. +// +// `mightContainDynamicImport` is the paired conservative pre-filter (the same +// accepted-risk shape as `mightReferencePackage`/`mightReferenceForbiddenRelativeDir` +// above): it decides whether a file is even worth handing to the expensive +// real-parser call, and it must never inspect or match the module-specifier +// text itself — only whether an `import` keyword could be followed by legal +// trivia (whitespace, a line comment, or a block comment) and then `(`. +// Ambiguity always resolves to `true` (send it to the parser); this is +// deliberately looser than a real grammar check (e.g. it does not verify +// `import` is being used as a call rather than, say, `import.meta`) because +// this gate's only job is to avoid the parser for source that PROVABLY has no +// dynamic import at all — any narrower attempt to also decide the argument's +// shape textually would reopen exactly the lexical-bypass risk this module's +// header comment already warns about. + +/** + * Classify every dynamic `import(...)` call expression in `source` — a real + * TypeScript parse, never a specifier-text regex. Every call expression whose + * callee is the bare `import` keyword contributes exactly one result; there + * is no `if (spec !== null) push(...)` shape here that could silently drop an + * unsupported argument (contrast `findModuleSpecifiers` above, whose whole + * point is the opposite: silently skip what it cannot resolve, because ITS + * callers have no fail-closed contract to uphold). + * + * @param {string} source + * @param {string} filename repo-relative, forward-slash separated (used only + * for the virtual-file basename/grammar selection) + * @returns {({kind: 'static', spec: string, pos: number} | {kind: 'uncheckable', pos: number})[]} + * `pos` is the call expression's own start offset (`node.getStart(sourceFile)`) + * — a stable identity a caller MAY use to de-duplicate the same occurrence + * across overlapping guarded-directory rules; it is not a line/column and + * is not required in any user-facing diagnostic. + */ +export function findDynamicImportUsages(source, filename) { + return withParsedSource(source, filename, (sourceFile) => { + const found = []; + const walk = (node) => { + if ( + is.isCallExpression(node) + && node.expression + && node.expression.kind === SyntaxKind.ImportKeyword + ) { + const pos = node.getStart(sourceFile); + const arg = node.arguments[0]; + if ( + arg + && (arg.kind === SyntaxKind.StringLiteral || arg.kind === SyntaxKind.NoSubstitutionTemplateLiteral) + ) { + found.push({ kind: 'static', spec: arg.text, pos }); + } else { + // Every other shape — a missing argument, an Identifier, a + // template literal WITH a substitution, a binary concatenation, a + // call, a conditional, a parenthesized/computed expression, or any + // future shape this list does not name — is uncheckable. There is + // deliberately no partial extraction attempt (e.g. reading a + // template literal's first quasi span): that is precisely the class + // of bug this issue exists to close (`import('../' + name)` must + // never be treated as `'../'`). + found.push({ kind: 'uncheckable', pos }); + } + } + node.forEachChild(walk); + }; + walk(sourceFile); + return found; + }); +} + +/** + * Cheap, deliberately over-inclusive textual pre-filter gating the expensive + * `findDynamicImportUsages` parse: true whenever `source` MIGHT contain a + * dynamic `import(...)` call — an `import` keyword, at a word boundary on + * both sides (so it never matches inside a longer identifier like + * `importFoo`), followed by any amount of ordinary whitespace/line-comment/ + * block-comment trivia and then an opening `(`. Never inspects or matches + * anything about the argument/specifier — that is exclusively + * `findDynamicImportUsages`'s job. A false positive (e.g. the literal text + * `"import("` sitting inside an unrelated string literal) merely costs one + * wasted parse that then correctly reports no dynamic-import call expression + * at all; a false negative would silently exempt a real dynamic import from + * ever reaching the parser, which this gate must never do. + * + * @param {string} source + * @returns {boolean} + */ +const DYNAMIC_IMPORT_GATE = /\bimport\b(?:[ \t\r\n]|\/\/[^\n]*(?:\n|$)|\/\*[\s\S]*?\*\/)*\(/; + +export function mightContainDynamicImport(source) { + return DYNAMIC_IMPORT_GATE.test(source); +} + /** Issue #630 Phase 8 (plan §19.1) — the plan's own generic vocabulary for * `findRetiredTopLevelApiViolations` above, which already generalizes over * an explicit `names` argument (it is not hardcoded to the Phase 7 retired diff --git a/tests/unit/check-boundaries-dynamic-imports.test.js b/tests/unit/check-boundaries-dynamic-imports.test.js new file mode 100644 index 00000000..2dc8aa80 --- /dev/null +++ b/tests/unit/check-boundaries-dynamic-imports.test.js @@ -0,0 +1,171 @@ +// Issue #642 — focused coverage for the shared parser-backed dynamic-import +// classifier (`findDynamicImportUsages`) and its conservative textual gate +// (`mightContainDynamicImport`), both in `build/lib/check-legacy-owners.mjs`. +// This is syntax-classification proof only, isolated from the two larger +// policy mirrors (`tests/unit/dashboard-boundaries.test.js` and +// `tests/unit/clickhouse-http-package-policy.test.js`) that apply an actual +// forbidden-directory/package policy on top of this classification. +// +// Node-tooling spec (kept .js like the other build-tooling specs in this +// tree: typing the node: imports would need @types/node, a deferred +// decision). + +import { describe, expect, it } from 'vitest'; +import { findDynamicImportUsages, mightContainDynamicImport } from '../../build/lib/check-legacy-owners.mjs'; + +const FILE = 'src/core/__probe__.ts'; + +describe('findDynamicImportUsages — static literal arguments', () => { + it("classifies import('../x.js') (single-quoted) as static with the decoded specifier", () => { + const found = findDynamicImportUsages("export async function f() { await import('../x.js'); }\n", FILE); + expect(found).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('classifies import("../x.js") (double-quoted) as static with the decoded specifier', () => { + const found = findDynamicImportUsages('export async function f() { await import("../x.js"); }\n', FILE); + expect(found).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('classifies import(`../x.js`) (no-substitution template) as static with the decoded specifier', () => { + const found = findDynamicImportUsages('export async function f() { await import(`../x.js`); }\n', FILE); + expect(found).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('classifies a static import with legal trivia around the call and argument', () => { + const found = findDynamicImportUsages( + "export async function f() { await import ( '../x.js' ); }\n", + FILE, + ); + expect(found).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('classifies a static import with a comment between "import" and its call parens', () => { + const found = findDynamicImportUsages( + "export async function f() { await import/*c*/('../x.js'); }\n", + FILE, + ); + expect(found).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('classifies a static import with a comment between the open paren and the specifier', () => { + const found = findDynamicImportUsages( + "export async function f() { await import(/*c*/'../x.js'); }\n", + FILE, + ); + expect(found).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); +}); + +describe('findDynamicImportUsages — uncheckable arguments (never no result, never null)', () => { + it('classifies a computed template literal (with a substitution) as uncheckable', () => { + const found = findDynamicImportUsages( + 'export async function f(name) { await import(`../${name}.js`); }\n', + FILE, + ); + expect(found).toEqual([{ kind: 'uncheckable', pos: expect.any(Number) }]); + }); + + it('classifies a bare identifier argument as uncheckable', () => { + const found = findDynamicImportUsages('export async function f(specifier) { await import(specifier); }\n', FILE); + expect(found).toEqual([{ kind: 'uncheckable', pos: expect.any(Number) }]); + }); + + it('classifies a concatenated argument as uncheckable — never reduced to its quoted prefix', () => { + const found = findDynamicImportUsages( + "export async function f(name) { await import('../' + name); }\n", + FILE, + ); + expect(found).toEqual([{ kind: 'uncheckable', pos: expect.any(Number) }]); + // Regression assertion: the result must never carry the quoted prefix as + // though it proved anything about the full runtime specifier. + expect(found.some((u) => u.spec === '../')).toBe(false); + }); + + it('classifies a conditional (ternary) expression argument as uncheckable', () => { + const found = findDynamicImportUsages( + "export async function f(cond) { await import(cond ? './a.js' : './b.js'); }\n", + FILE, + ); + expect(found).toEqual([{ kind: 'uncheckable', pos: expect.any(Number) }]); + }); + + it('classifies a parenthesized/computed expression argument as uncheckable', () => { + const found = findDynamicImportUsages( + "export async function f(name) { await import((name)); }\n", + FILE, + ); + expect(found).toEqual([{ kind: 'uncheckable', pos: expect.any(Number) }]); + }); + + it('classifies a call-expression argument as uncheckable', () => { + const found = findDynamicImportUsages( + "export async function f(resolve) { await import(resolve('x')); }\n", + FILE, + ); + expect(found).toEqual([{ kind: 'uncheckable', pos: expect.any(Number) }]); + }); + + it('classifies a missing argument as uncheckable rather than throwing', () => { + const found = findDynamicImportUsages('export async function f() { await import(); }\n', FILE); + expect(found).toEqual([{ kind: 'uncheckable', pos: expect.any(Number) }]); + }); +}); + +describe('findDynamicImportUsages — multiple occurrences and clean source', () => { + it('reports one result per dynamic-import call expression, in source order', () => { + const probe = ` + export async function f(name) { + await import('../a.js'); + await import(name); + } + `; + const found = findDynamicImportUsages(probe, FILE); + expect(found).toHaveLength(2); + expect(found[0]).toEqual({ kind: 'static', spec: '../a.js', pos: expect.any(Number) }); + expect(found[1]).toEqual({ kind: 'uncheckable', pos: expect.any(Number) }); + }); + + it('returns an empty array for source with no dynamic import at all', () => { + expect(findDynamicImportUsages("import { x } from './x.js';\nexport const y = x;\n", FILE)).toEqual([]); + }); + + it('does not classify a static import/export declaration as a dynamic import', () => { + expect(findDynamicImportUsages("import './x.js';\nexport * from './y.js';\n", FILE)).toEqual([]); + }); +}); + +describe('mightContainDynamicImport — conservative gate, never inspects the argument', () => { + it('returns true for a plain dynamic import', () => { + expect(mightContainDynamicImport("import('x')")).toBe(true); + }); + + it('returns true with whitespace/newlines before the call parens', () => { + expect(mightContainDynamicImport('import\n \t (\'x\')')).toBe(true); + }); + + it('returns true with a block comment between "import" and its call parens', () => { + expect(mightContainDynamicImport("import/*comment*/('x')")).toBe(true); + }); + + it('returns true with a line comment between "import" and its call parens', () => { + expect(mightContainDynamicImport('import // comment\n (\'x\')')).toBe(true); + }); + + it('returns false for source with no dynamic import at all', () => { + expect(mightContainDynamicImport("import { x } from './x.js';\nexport const y = x;\n")).toBe(false); + }); + + it('returns false for a source that merely mentions the word "import" without a call', () => { + expect(mightContainDynamicImport('// see the import graph in docs/ARCHITECTURE.md\nexport const z = 1;\n')).toBe(false); + }); + + it('does not match "import" as a suffix of a longer identifier (e.g. reimport)', () => { + expect(mightContainDynamicImport('function reimport() {}\nreimport();\n')).toBe(false); + }); + + it('returns true (harmless false positive) for a string literal that merely spells "import(" — proving the parser, not the gate, stays authoritative', () => { + expect(mightContainDynamicImport('const s = "please call import(fn) sometime";\n')).toBe(true); + // And the real parser correctly finds no dynamic-import call at all. + expect(findDynamicImportUsages('const s = "please call import(fn) sometime";\n', FILE)).toEqual([]); + }); +}); diff --git a/tests/unit/clickhouse-http-package-policy.test.js b/tests/unit/clickhouse-http-package-policy.test.js index c4f0300e..f719a51e 100644 --- a/tests/unit/clickhouse-http-package-policy.test.js +++ b/tests/unit/clickhouse-http-package-policy.test.js @@ -71,6 +71,8 @@ import { findTransportSurfaceOwnershipViolations, PHASE8_TRANSPORT_SURFACE_NAMES, PHASE8_PARSER_SURFACE_NAMES, + findDynamicImportUsages, + mightContainDynamicImport, } from '../../build/lib/check-legacy-owners.mjs'; const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), '../..'); @@ -92,21 +94,27 @@ function collectFiles(dir) { return out; } -// Same four-pattern specifier scan as build/check-boundaries.mjs's own -// `extractSpecifiers` (static import/export-from, bare side-effect import, -// dynamic import) — independently implemented, not imported (importing the +// Same three-pattern STATIC specifier scan as build/check-boundaries.mjs's +// own `extractStaticSpecifiers` (static import/export-from, bare side-effect +// import) — independently implemented, not imported (importing the // production script would run its whole top-level check-and-exit routine -// inside this test process). The dynamic-import pattern also accepts a -// backtick no-substitution template literal, matching the production -// scanner (only a dynamic `import(...)` call can syntactically take one). +// inside this test process). +// +// Issue #642 — the fourth pattern this array used to carry (a dynamic +// `import(...)` regex) is gone: it could only ever match an argument that +// LOOKED like a complete literal, silently missing a computed dynamic import +// entirely or, on a concatenated expression, matching just its quoted prefix. +// Rule A/Rule B below now classify every dynamic import through the shared +// real-parser helper `findDynamicImportUsages` instead (see +// `dynamicImportUsagesFor` just below), the same fail-closed contract +// `build/check-boundaries.mjs`'s own generic RULES pre-pass now uses. const SPECIFIER_PATTERNS = [ /\bimport\s+[\w*{}\s,]+\s+from\s*['"]([^'"]+)['"]/g, /\bexport\s+[\w*{}\s,]+\s+from\s*['"]([^'"]+)['"]/g, /\bimport\s*['"]([^'"]+)['"]/g, - /\bimport\s*\(\s*[`'"]([^`'"]+)[`'"]/g, ]; -function extractSpecifiers(source) { +function extractStaticSpecifiers(source) { const specs = []; for (const pattern of SPECIFIER_PATTERNS) { pattern.lastIndex = 0; @@ -116,6 +124,17 @@ function extractSpecifiers(source) { return specs; } +// Issue #642 — the shared parser-backed dynamic-import classification, gated +// by the same conservative textual pre-filter production uses +// (`mightContainDynamicImport`), for Rule A/Rule B only (Rule C/Rule D below +// already have their own dedicated real-parser dynamic-import handling and +// are untouched by this issue). Returns `[]` (never calls the parser at all) +// when the file provably contains no `import(...)` call. +function dynamicImportUsagesFor(file, text, relFile) { + if (!mightContainDynamicImport(text)) return []; + return findDynamicImportUsages(text, relFile); +} + // Mirrors production's `resolveRelative` (`build/check-boundaries.mjs`), // including the review-pass-2 symlink-canonicalization fix: `fs.realpathSync` // on any candidate that exists resolves the real, already-installed @@ -153,20 +172,43 @@ function collectEntries(dir, virtualFiles = []) { // Rule A mirror: relative specifiers resolving into a forbidden directory. // Review pass 1: this regex-based mirror now covers ONLY Rule A (production -// still legitimately scans that one with `extractSpecifiers`, same as the -// other internal src-layering rules) — Rule C/Guard 2 moved to its own +// still legitimately scans that one with `extractStaticSpecifiers`, same as +// the other internal src-layering rules) — Rule C/Guard 2 moved to its own // dedicated real-parser block in production and has its own parser-backed // mirror, `relativeViolationsParserBacked`, below. +// +// Issue #642 — Rule A now also classifies every dynamic `import(...)` call +// through the shared real-parser helper: a `static` one is resolved and +// checked exactly like an ordinary static import; an `uncheckable` one +// (identifier, computed template, concatenation, conditional, or any other +// non-literal argument shape) is an unconditional finding on its own, +// independent of where it might have resolved. `packages/clickhouse-http/src` +// is itself a generic-guarded tree, so this is the "package-source computed +// dynamic import" fail-closed proof for Rule A/Rule B (see the Assumption in +// the approved plan: Rule B needs no independent uncheckable detection of its +// own, since Rule A's mirror over the same directory already covers it). function relativeViolations(dir, forbidden, virtualFiles = []) { const found = []; for (const [file, source] of collectEntries(dir, virtualFiles)) { const text = source ?? readFileSync(file, 'utf8'); - for (const spec of extractSpecifiers(text)) { + const relFile = relative(repoRoot, file).split(sep).join('/'); + for (const spec of extractStaticSpecifiers(text)) { if (!spec.startsWith('.')) continue; const resolved = resolveRelative(file, spec); const relResolved = relative(repoRoot, resolved).split(sep).join('/'); const hit = forbidden.find((f) => relResolved === f || relResolved.startsWith(`${f}/`)); - if (hit) found.push(`${relative(repoRoot, file).split(sep).join('/')} → ${spec} (resolved: ${relResolved})`); + if (hit) found.push(`${relFile} → ${spec} (resolved: ${relResolved})`); + } + for (const usage of dynamicImportUsagesFor(file, text, relFile)) { + if (usage.kind === 'uncheckable') { + found.push(`${relFile} → dynamic import(...) (uncheckable)`); + continue; + } + if (!usage.spec.startsWith('.')) continue; + const resolved = resolveRelative(file, usage.spec); + const relResolved = relative(repoRoot, resolved).split(sep).join('/'); + const hit = forbidden.find((f) => relResolved === f || relResolved.startsWith(`${f}/`)); + if (hit) found.push(`${relFile} → ${usage.spec} (resolved: ${relResolved})`); } } return found; @@ -174,14 +216,14 @@ function relativeViolations(dir, forbidden, virtualFiles = []) { // Rule C mirror (Guard 2, review pass 1): parser-backed (`findModuleSpecifiers`), // matching production's dedicated Guard 2 block in `build/check-boundaries.mjs` -// — NOT `relativeViolations`'s `extractSpecifiers` regex above, which stayed -// vulnerable to a comment sitting between `import`/`export` and the +// — NOT `relativeViolations`'s `extractStaticSpecifiers` regex above, which +// stayed vulnerable to a comment sitting between `import`/`export` and the // specifier, or an escaped string-literal segment spelling out a // `packages/clickhouse-http` path without ever containing that raw -// substring: `extractSpecifiers` captures the raw, still-escaped/comment- -// adjacent source text, which then fails to resolve into the forbidden -// directory, so the escape silently slipped this guard entirely before this -// fix. Memoized the on-disk component the same way `deepImportViolations` +// substring: `extractStaticSpecifiers` captures the raw, still-escaped/ +// comment-adjacent source text, which then fails to resolve into the +// forbidden directory, so the escape silently slipped this guard entirely +// before this fix. Memoized the on-disk component the same way `deepImportViolations` // above is: the real `src/` tree never changes within one test-file run, but // every Rule C test below re-passes `join(repoRoot, 'src')` with a DIFFERENT // single virtual sabotage probe appended, and re-spawning the real @@ -240,13 +282,25 @@ function relativeViolationsParserBacked(dir, forbidden, virtualFiles = []) { // violation (empty bare-specifier allowlist) — this also naturally catches // a browser-root-literal import like `/src/net/ch-client.js`, which is not // a relative specifier either. +// +// Issue #642 — Rule B needs no independent `uncheckable` detection: package +// source is ALSO Rule A's guarded directory, so a computed dynamic import +// there already fails closed via `relativeViolations` above (see that +// function's own comment). What Rule B still owns is exactly the bare-vs- +// relative policy decision for a dynamic import whose specifier IS statically +// known — reusing the SAME shared classifier, not a second hand-rolled regex. function bareSpecifierViolations(dir, virtualFiles = []) { const found = []; for (const [file, source] of collectEntries(dir, virtualFiles)) { const text = source ?? readFileSync(file, 'utf8'); - for (const spec of extractSpecifiers(text)) { + const relFile = relative(repoRoot, file).split(sep).join('/'); + for (const spec of extractStaticSpecifiers(text)) { if (spec.startsWith('.')) continue; - found.push(`${relative(repoRoot, file).split(sep).join('/')} → ${spec}`); + found.push(`${relFile} → ${spec}`); + } + for (const usage of dynamicImportUsagesFor(file, text, relFile)) { + if (usage.kind !== 'static' || usage.spec.startsWith('.')) continue; + found.push(`${relFile} → ${usage.spec}`); } } return found; @@ -447,6 +501,70 @@ describe('Rule A — package source imports no SQL Browser src/** (relative)', ( ]); expect(found.some((line) => line.includes('__boundary_probe_630__') && line.includes('src/application'))).toBe(true); }); + + // Issue #642 — dynamic import(...) fails closed for Rule A too: package + // source is itself a generic-guarded tree. All three statically analyzable + // delimiter forms remain reachable through the ordinary relative/forbidden + // policy; anything else is uncheckable regardless of where it might resolve. + it('flags a single-quoted dynamic import reaching into src/application (issue #642, sabotage probe, not written to disk)', () => { + const found = relativeViolations(PACKAGE_SRC_DIR, ['src'], [ + ['packages/clickhouse-http/src/__boundary_probe_642_single__.ts', + "export async function f() { await import('../../../src/application/does-not-exist.js'); }\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_single__') && line.includes('src/application'))).toBe(true); + }); + + it('flags a double-quoted dynamic import reaching into src/application (issue #642, sabotage probe, not written to disk)', () => { + const found = relativeViolations(PACKAGE_SRC_DIR, ['src'], [ + ['packages/clickhouse-http/src/__boundary_probe_642_double__.ts', + 'export async function f() { await import("../../../src/application/does-not-exist.js"); }\n'], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_double__') && line.includes('src/application'))).toBe(true); + }); + + it('flags a no-substitution-template dynamic import reaching into src/application (issue #642, sabotage probe, not written to disk)', () => { + const found = relativeViolations(PACKAGE_SRC_DIR, ['src'], [ + ['packages/clickhouse-http/src/__boundary_probe_642_template__.ts', + 'export async function f() { await import(`../../../src/application/does-not-exist.js`); }\n'], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_template__') && line.includes('src/application'))).toBe(true); + }); + + // The exact package-source computed-dynamic-import sabotage the approved + // plan requires: this mirror must not silently accept a computed dynamic + // import under packages/clickhouse-http/src/**. + it('flags a computed template-literal dynamic import in package source as uncheckable (issue #642, sabotage probe, not written to disk)', () => { + const found = relativeViolations(PACKAGE_SRC_DIR, ['src'], [ + ['packages/clickhouse-http/src/__boundary_probe_642_computed__.ts', + 'export async function f(name) { await import(`../../../src/application/${name}.js`); }\n'], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_computed__') && line.includes('uncheckable'))).toBe(true); + }); + + it('flags an identifier-argument dynamic import in package source as uncheckable (issue #642, sabotage probe, not written to disk)', () => { + const found = relativeViolations(PACKAGE_SRC_DIR, ['src'], [ + ['packages/clickhouse-http/src/__boundary_probe_642_identifier__.ts', + 'export async function f(spec) { await import(spec); }\n'], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_identifier__') && line.includes('uncheckable'))).toBe(true); + }); + + it('flags a concatenated dynamic import in package source as uncheckable — never reduced to its quoted prefix (issue #642, sabotage probe, not written to disk)', () => { + const found = relativeViolations(PACKAGE_SRC_DIR, ['src'], [ + ['packages/clickhouse-http/src/__boundary_probe_642_concat__.ts', + "export async function f(name) { await import('../../../src/' + name); }\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_concat__') && line.includes('uncheckable'))).toBe(true); + expect(found.some((line) => line.includes('__boundary_probe_642_concat__') && line.includes("→ '../../../src/'"))).toBe(false); + }); + + it('does not flag a legal relative dynamic import within the package itself (issue #642)', () => { + const found = relativeViolations(PACKAGE_SRC_DIR, ['src'], [ + ['packages/clickhouse-http/src/__boundary_probe_642_legal__.ts', + "export async function f() { await import('./client.js'); }\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_legal__'))).toBe(false); + }); }); describe('Rule B — package source has zero bare specifiers (empty allowlist)', () => { @@ -474,6 +592,25 @@ describe('Rule B — package source has zero bare specifiers (empty allowlist)', ]); expect(found.some((line) => line.includes('__boundary_probe_630_literal__') && line.includes('/src/net/ch-client.js'))).toBe(true); }); + + // Issue #642 — Rule B still owns the bare-vs-relative policy decision for a + // dynamic import whose specifier IS statically known (the old dynamic regex + // arm this replaces used to catch exactly this case). + it('flags a bare dynamic import of a package specifier (static form, issue #642, sabotage probe, not written to disk)', () => { + const found = bareSpecifierViolations(PACKAGE_SRC_DIR, [ + ['packages/clickhouse-http/src/__boundary_probe_642_baredynamic__.ts', + "export async function f() { await import('left-pad'); }\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_baredynamic__') && line.includes('left-pad'))).toBe(true); + }); + + it('does not flag a relative dynamic import (governed by Rule A instead, issue #642)', () => { + const found = bareSpecifierViolations(PACKAGE_SRC_DIR, [ + ['packages/clickhouse-http/src/__boundary_probe_642_bare_relative__.ts', + "export async function f() { await import('./client.js'); }\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_bare_relative__'))).toBe(false); + }); }); // Issue #630 Phase 8 (plan §21, Guard 2) broadens Rule C's forbidden target @@ -1200,6 +1337,31 @@ describe('build/check-boundaries.mjs still declares the Rules A-D this spec mirr expect(checkerSource).toMatch(/clickhouse-http has zero bare package imports/); }); + // Issue #642 — the generic RULES loop (which Rule A above lives inside) + // and Rule B must both classify dynamic imports through the shared + // parser-backed helper and its conservative gate, and the old dynamic- + // import regex arm that used to live in the checker's SPECIFIER_PATTERNS + // must be gone entirely — not just from Rule A/B's own blocks, but from the + // whole file (there is no legitimate reason for it to survive anywhere, + // since every dynamic import in a generic-guarded file is now classified + // by the real parser). + it('the generic RULES loop and Rule B fail closed on computed dynamic imports via the shared classifier, not a reintroduced regex (issue #642)', () => { + const importBlock = checkerSource.match(/import \{([^}]*)\} from '\.\/lib\/check-legacy-owners\.mjs';/); + expect(importBlock, 'check-legacy-owners import block missing from build/check-boundaries.mjs').not.toBeNull(); + expect(importBlock[1]).toMatch(/\bfindDynamicImportUsages\b/); + expect(importBlock[1]).toMatch(/\bmightContainDynamicImport\b/); + expect(checkerSource).toMatch(/findDynamicImportUsages\(/); + expect(checkerSource).toMatch(/mightContainDynamicImport\(/); + expect(checkerSource).toMatch(/cannot be statically checked against the architecture boundary/); + // The renamed static-only scanner replaces the old `extractSpecifiers` + // name everywhere in production; the removed dynamic-import regex + // pattern (`[`'"]([^`'"]+)[`'"]` following an `import\s*\(` prefix) must + // not exist anywhere in the file, not merely be absent from Rule A/B. + expect(checkerSource).toMatch(/function extractStaticSpecifiers\(/); + expect(checkerSource).not.toMatch(/function extractSpecifiers\(/); + expect(checkerSource.includes("[`'\"]([^`'\"]+)[`'\"]")).toBe(false); + }); + // Issue #630 Phase 8 (plan §21, Guard 2) broadened Rule C's forbidden // target from just `packages/clickhouse-http/src` to the whole package // directory (`packages/clickhouse-http`) — dist escape coverage. Review diff --git a/tests/unit/dashboard-boundaries.test.js b/tests/unit/dashboard-boundaries.test.js index 55d61cd8..22fb7a3b 100644 --- a/tests/unit/dashboard-boundaries.test.js +++ b/tests/unit/dashboard-boundaries.test.js @@ -12,6 +12,7 @@ import { describe, expect, it } from 'vitest'; import { readdirSync, readFileSync, existsSync, statSync } from 'node:fs'; import { dirname, join, relative, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; +import { findDynamicImportUsages, mightContainDynamicImport } from '../../build/lib/check-legacy-owners.mjs'; const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), '../..'); const SOURCE_EXT = /\.(ts|tsx|js|mjs)$/; @@ -78,6 +79,26 @@ function violations(dir, forbidden = FORBIDDEN, virtualFiles = []) { const hit = forbidden.find((f) => resolved === f || resolved.startsWith(`${f}/`)); if (hit) found.push(`${relative(repoRoot, file)} → ${spec} (${hit})`); } + // Issue #642: combine the existing static relative-specifier path above + // with the shared parser-backed dynamic-import classifier — a `static` + // dynamic import is resolved and checked exactly like an ordinary + // relative import; an `uncheckable` one (identifier, computed template, + // concatenation, conditional, or any other non-literal argument shape) + // is an explicit, unconditional finding, independent of where it might + // have resolved. + const text = source ?? readFileSync(file, 'utf8'); + if (!mightContainDynamicImport(text)) continue; + const relFile = relative(repoRoot, file).split('\\').join('/'); + for (const usage of findDynamicImportUsages(text, relFile)) { + if (usage.kind === 'uncheckable') { + found.push(`${relative(repoRoot, file)} → dynamic import(...) (uncheckable)`); + continue; + } + if (!usage.spec.startsWith('.')) continue; // bare/package specifiers can't reach the forbidden dirs + const resolved = resolveSpec(file, usage.spec); + const hit = forbidden.find((f) => resolved === f || resolved.startsWith(`${f}/`)); + if (hit) found.push(`${relative(repoRoot, file)} → ${usage.spec} (${hit})`); + } } return found; } @@ -145,6 +166,120 @@ describe('dashboard dependency boundaries', () => { expect(forbidden.slice().sort()).toEqual(FORBIDDEN_CORE.slice().sort()); }); + // Issue #642 — computed/non-static dynamic imports must fail closed under + // the same generic guarded-directory rule a static import already obeys. + // Every probe below is virtual (never written to disk), matching this + // file's own #554 convention. + describe('issue #642 — dynamic import(...) fails closed for src/core (mirrors build/check-boundaries.mjs)', () => { + it('flags a single-quoted dynamic import into src/workspace', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_single__.ts', + "export async function f() { await import('../workspace/does-not-exist.js'); }\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_single__') && line.includes('src/workspace'))).toBe(true); + }); + + it('flags a double-quoted dynamic import into src/workspace', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_double__.ts', + 'export async function f() { await import("../workspace/does-not-exist.js"); }\n'], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_double__') && line.includes('src/workspace'))).toBe(true); + }); + + it('flags a no-substitution-template dynamic import into src/workspace', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_template__.ts', + 'export async function f() { await import(`../workspace/does-not-exist.js`); }\n'], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_template__') && line.includes('src/workspace'))).toBe(true); + }); + + it('rejects a computed template-literal dynamic import as uncheckable', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_computed__.ts', + 'export async function f(name) { await import(`../workspace/${name}.js`); }\n'], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_computed__') && line.includes('uncheckable'))).toBe(true); + }); + + it('rejects an identifier-argument dynamic import as uncheckable', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_identifier__.ts', + 'export async function f(specifier) { await import(specifier); }\n'], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_identifier__') && line.includes('uncheckable'))).toBe(true); + }); + + it('rejects a concatenated dynamic import as uncheckable — never reduced to its quoted prefix', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_concat__.ts', + "export async function f(name) { await import('../workspace/' + name); }\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_concat__') && line.includes('uncheckable'))).toBe(true); + // Regression: must never resolve to (or report) the quoted prefix alone. + expect(found.some((line) => line.includes('__boundary_probe_642_concat__') && line.includes("→ '../workspace/'"))).toBe(false); + }); + + it('rejects a conditional-expression dynamic import as uncheckable', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_conditional__.ts', + "export async function f(cond) { await import(cond ? '../workspace/a.js' : '../workspace/b.js'); }\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_conditional__') && line.includes('uncheckable'))).toBe(true); + }); + + it('accepts a direct dynamic import resolving within an allowed layer', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_legal__.ts', + "export async function f() { await import('./format.js'); }\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_legal__'))).toBe(false); + }); + + // Regression coverage for the required static export forms — these must + // continue through the existing static fast path, not the new dynamic + // parser helper, and must still be rejected when they cross the boundary. + it('flags export * from crossing a forbidden boundary (regression)', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_exportstar__.ts', "export * from '../workspace/does-not-exist.js';\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_exportstar__') && line.includes('src/workspace'))).toBe(true); + }); + + it('flags export { name } from crossing a forbidden boundary (regression)', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_exportnamed__.ts', "export { nothing } from '../workspace/does-not-exist.js';\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_exportnamed__') && line.includes('src/workspace'))).toBe(true); + }); + + it('flags export * as ns from crossing a forbidden boundary (regression)', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_exportnamespace__.ts', "export * as ns from '../workspace/does-not-exist.js';\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_exportnamespace__') && line.includes('src/workspace'))).toBe(true); + }); + + // Drift bind: proves `build/check-boundaries.mjs` itself — not just this + // mirror — actually classifies dynamic imports through the shared + // real-parser helpers and no longer carries the old dynamic-import regex + // arm this issue retires. + it('build/check-boundaries.mjs classifies dynamic imports via the shared parser-backed helper and fails closed (#642)', () => { + const checkerSource = readFileSync(join(repoRoot, 'build/check-boundaries.mjs'), 'utf8'); + const importBlock = checkerSource.match(/import \{([^}]*)\} from '\.\/lib\/check-legacy-owners\.mjs';/); + expect(importBlock, 'check-legacy-owners import block missing from build/check-boundaries.mjs').not.toBeNull(); + expect(importBlock[1]).toMatch(/\bfindDynamicImportUsages\b/); + expect(importBlock[1]).toMatch(/\bmightContainDynamicImport\b/); + expect(checkerSource).toMatch(/findDynamicImportUsages\(/); + expect(checkerSource).toMatch(/mightContainDynamicImport\(/); + expect(checkerSource).toMatch(/cannot be statically checked against the architecture boundary/); + // The old dynamic-import regex arm (matching a `` ` ``/'/" -delimited + // argument directly against a `\bimport\s*\(` prefix) must be gone. + expect(checkerSource.includes("[`'\"]([^`'\"]+)[`'\"]")).toBe(false); + }); + }); + it('does not restore the retired saved-query repair planner or its vocabulary', () => { const retiredPath = ['saved-query', 'mutation.ts'].join('-'); expect(existsSync(join(repoRoot, 'src/dashboard/application', retiredPath))).toBe(false); From 650378dac12120d1618644f65659bee37b377e6e Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Mon, 10 Aug 2026 15:50:05 +0200 Subject: [PATCH 2/3] fix(#642): close CR/Unicode-trivia gap in mightContainDynamicImport DYNAMIC_IMPORT_GATE only recognized ASCII space/tab/CR/LF as whitespace and only backslash-n as a line-comment terminator, so a bare CR (not followed by LF), U+2028 LINE SEPARATOR, U+2029 PARAGRAPH SEPARATOR, or vertical-tab/form-feed whitespace between "import" and its call parens made the gate return false for source the real parser correctly classifies as containing a dynamic import - silently exempting the file from check-boundaries.mjs's fail-closed pre-pass entirely, the same fail-open prefilter pattern this issue exists to close, just relocated into the new gate. Rewrite the gate's whitespace alternative to use regex \s (ECMA-262 defines this to match exactly the union of WhiteSpace and LineTerminator code points, so it is sound by construction rather than by enumerating individual code points one at a time), and give the line-comment alternative its own explicit LineTerminator class, written in source with regex escape sequences for U+2028 and U+2029 rather than raw characters, at both its "still inside the comment" and "ends the comment" positions - \s itself would incorrectly let a plain space terminate a line comment early. Adds regression coverage for each gap (bare CR, U+2028, U+2029, vertical tab, form feed, and comment termination at U+2028) and re-verifies live: a CR-comment probe file under src/core now correctly fails check:arch, matching the LF-terminated form; both were confirmed and cleaned up before this commit. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- build/lib/check-legacy-owners.mjs | 36 ++++++++++++- .../check-boundaries-dynamic-imports.test.js | 51 +++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/build/lib/check-legacy-owners.mjs b/build/lib/check-legacy-owners.mjs index 2597d84c..68cedb83 100644 --- a/build/lib/check-legacy-owners.mjs +++ b/build/lib/check-legacy-owners.mjs @@ -553,10 +553,44 @@ export function findDynamicImportUsages(source, filename) { * at all; a false negative would silently exempt a real dynamic import from * ever reaching the parser, which this gate must never do. * + * Issue #642 review — an earlier revision of this gate hand-rolled its trivia + * character classes (`[ \t\r\n]` for whitespace, `[^\n]*(?:\n|$)` for a + * line-comment's extent) and only covered the ASCII subset of what + * ECMAScript's own grammar treats as WhiteSpace/LineTerminator: real + * LineTerminators also include a bare CR (not followed by LF), U+2028 LINE + * SEPARATOR, and U+2029 PARAGRAPH SEPARATOR, and real WhiteSpace also + * includes VT (`\v`), FF (`\f`), NBSP, ZWNBSP, and every other Unicode + * `Space_Separator` code point — none of which `[ \t\r\n]`/`[^\n]` covered, + * so a comment or run of whitespace built from one of them made the gate + * return `false` for source the real parser (correctly) sees as containing a + * dynamic import, exempting that file from ever reaching the fail-closed + * check this issue exists to add. Rather than chase individual code points + * one at a time (the same trap that produced the gap), this uses regex `\s` + * for the whitespace/line-terminator alternative: `\s` is not an + * approximation here, it is ECMA-262-DEFINED to match exactly the union of + * WhiteSpace and LineTerminator code points (`\t\n\v\f\r` plus the space + * character, NBSP, ZWNBSP/BOM, U+2028, U+2029, and the rest of Unicode + * `Space_Separator`) regardless of the `u`/`v` flag, so it is sound by + * construction rather than by enumeration. The line-comment alternative + * separately needs its own explicit LineTerminator class, spelled with + * literal `\u2028`/`\u2029` regex escapes (never raw characters, so the + * source itself stays free of invisible/hard-to-diff code points) — at both + * its "not part of the comment" and "ends the comment" positions. `\s` + * itself is unsuitable there because it also matches plain whitespace (an + * ordinary space does NOT end a `//` comment, only a real LineTerminator + * does), so reusing `\s` for that spot would have plain spaces terminate + * the comment early instead of extending it. (There is no cheaper + * alternative to a regex here: this module's own header comment already + * establishes that typescript@7 ships no in-process JS scanner/parser to + * delegate trivia-skipping to — every parse, including a trivia-only one, + * would mean spawning the native `tsc` child process this gate exists + * specifically to avoid paying for on every file.) + * * @param {string} source * @returns {boolean} */ -const DYNAMIC_IMPORT_GATE = /\bimport\b(?:[ \t\r\n]|\/\/[^\n]*(?:\n|$)|\/\*[\s\S]*?\*\/)*\(/; +const DYNAMIC_IMPORT_GATE = + /\bimport\b(?:\s|\/\/[^\n\r\u2028\u2029]*(?:[\n\r\u2028\u2029]|$)|\/\*[\s\S]*?\*\/)*\(/; export function mightContainDynamicImport(source) { return DYNAMIC_IMPORT_GATE.test(source); diff --git a/tests/unit/check-boundaries-dynamic-imports.test.js b/tests/unit/check-boundaries-dynamic-imports.test.js index 2dc8aa80..7e1fedab 100644 --- a/tests/unit/check-boundaries-dynamic-imports.test.js +++ b/tests/unit/check-boundaries-dynamic-imports.test.js @@ -169,3 +169,54 @@ describe('mightContainDynamicImport — conservative gate, never inspects the ar expect(findDynamicImportUsages('const s = "please call import(fn) sometime";\n', FILE)).toEqual([]); }); }); + +describe('mightContainDynamicImport — real ECMAScript trivia beyond ASCII space/tab/CR/LF', () => { + // Issue #642 review — the gate's original regex only recognized ASCII + // space/tab/CR/LF as whitespace and only `\n` as a line-comment + // terminator. Real ECMAScript trivia is broader: LineTerminator also + // includes a bare CR (not followed by LF), U+2028 LINE SEPARATOR, and + // U+2029 PARAGRAPH SEPARATOR; WhiteSpace also includes `\v`, `\f`, and + // other Unicode space separators. Each case below is a shape the real + // parser (`findDynamicImportUsages`) correctly classifies as a dynamic + // import — the gate must never return `false` for source it is fed, + // or `check-boundaries.mjs`'s pre-pass silently skips the parser call + // entirely and the file escapes the fail-closed check outright. + + it('returns true for a bare-CR-terminated line comment between "import" and its call parens (not CRLF)', () => { + const src = 'export async function f(specifier) { await import //c\r(specifier); }\n'; + expect(mightContainDynamicImport(src)).toBe(true); + // Cross-check against the real parser, matching this issue's own live + // reproduction: it must find a real (uncheckable) dynamic import here. + expect(findDynamicImportUsages(src, FILE)).toEqual([{ kind: 'uncheckable', pos: expect.any(Number) }]); + }); + + it('returns true for U+2028 LINE SEPARATOR as whitespace between "import" and its call parens', () => { + const src = `export async function f() { await import${'\u2028'}('../x.js'); }\n`; + expect(mightContainDynamicImport(src)).toBe(true); + expect(findDynamicImportUsages(src, FILE)).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('returns true for U+2029 PARAGRAPH SEPARATOR as whitespace between "import" and its call parens', () => { + const src = `export async function f() { await import${'\u2029'}('../x.js'); }\n`; + expect(mightContainDynamicImport(src)).toBe(true); + expect(findDynamicImportUsages(src, FILE)).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('returns true for a vertical tab (\\v) as whitespace between "import" and its call parens', () => { + const src = `export async function f() { await import${'\v'}('../x.js'); }\n`; + expect(mightContainDynamicImport(src)).toBe(true); + expect(findDynamicImportUsages(src, FILE)).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('returns true for a form feed (\\f) as whitespace between "import" and its call parens', () => { + const src = `export async function f() { await import${'\f'}('../x.js'); }\n`; + expect(mightContainDynamicImport(src)).toBe(true); + expect(findDynamicImportUsages(src, FILE)).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('still terminates a line comment at U+2028, not just \\n, so a call after it is still seen', () => { + const src = `export async function f() { await import //c${'\u2028'}('../x.js'); }\n`; + expect(mightContainDynamicImport(src)).toBe(true); + expect(findDynamicImportUsages(src, FILE)).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); +}); From a2318aa2612ab65f1f059649ad51506744b667d4 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Mon, 10 Aug 2026 16:26:45 +0200 Subject: [PATCH 3/3] fix(#642): address review pass 1 findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit findDynamicImportUsages only walked CallExpression import(...) nodes, so TypeScript's inline import-type expression (`type T = import('x').Foo`, `typeof import('x')`) — its own ImportTypeNode grammar production — silently bypassed the generic RULES loop and Rule B fail-closed pre-pass in build/check-boundaries.mjs, even though mightContainDynamicImport's textual gate correctly let those files through. The retired regex this issue replaced matched this shape too (it couldn't distinguish a call from an import-type expression either), so the AST-based classifier had strictly less coverage for this one form until now. findDynamicImportUsages now also walks ImportTypeNode, classifying its literal argument through the same {kind: 'static'|'uncheckable'} contract as a dynamic call. Added unit coverage in check-boundaries-dynamic-imports.test.js plus sabotage probes in dashboard-boundaries.test.js and clickhouse-http-package-policy.test.js proving the fix at the RULES-loop/ Rule-A/Rule-B integration level (verified failing without the production fix). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- build/check-boundaries.mjs | 35 +++--- build/lib/check-legacy-owners.mjs | 105 ++++++++++++------ .../check-boundaries-dynamic-imports.test.js | 48 ++++++++ .../clickhouse-http-package-policy.test.js | 53 +++++++++ tests/unit/dashboard-boundaries.test.js | 38 +++++++ 5 files changed, 233 insertions(+), 46 deletions(-) diff --git a/build/check-boundaries.mjs b/build/check-boundaries.mjs index bde2beaf..c351001b 100644 --- a/build/check-boundaries.mjs +++ b/build/check-boundaries.mjs @@ -50,21 +50,26 @@ // were absent. Every generic-guarded file is now additionally classified by // the shared real-parser helpers `findDynamicImportUsages`/ // `mightContainDynamicImport` below — `mightContainDynamicImport` gates the -// cheap case (a file that provably contains no `import(...)` call skips the -// parser entirely, preserving the ordinary static fast path for files with no -// dynamic import at all); `findDynamicImportUsages` then classifies every -// dynamic-import call expression in a matched file as `{ kind: 'static', -// spec }` (a plain string/no-substitution-template-literal argument, fed -// through the exact same relative-resolution/forbidden-prefix logic as an -// ordinary static import) or `{ kind: 'uncheckable' }` (everything else — -// identifier, computed template, concatenation, conditional, or any other -// expression shape — an UNCONDITIONAL violation, independent of which rule -// eventually would have matched the file). Computed/non-static dynamic -// imports are forbidden in every source file covered by at least one generic -// `RULES` entry; Rule C/Rule D and the other already-parser-backed package -// guards are untouched by this — they already classify their own dynamic -// imports through the real parser and #630/#646/#653's package-guard work is -// not being redone here. +// cheap case (a file that provably contains no `import(...)`-shaped +// construct skips the parser entirely, preserving the ordinary static fast +// path for files with none at all); `findDynamicImportUsages` then classifies +// every dynamic-import call expression AND every TypeScript inline +// import-type expression (`type T = import('x').Foo`, `typeof import('x')` — +// review pass 1: a structurally distinct `ImportTypeNode`, textually +// identical at the `import(...)` shape the gate above matches, so it reaches +// this classifier too and must not silently fall through it) in a matched +// file as `{ kind: 'static', spec }` (a plain string/no-substitution- +// template-literal argument, fed through the exact same relative-resolution/ +// forbidden-prefix logic as an ordinary static import) or +// `{ kind: 'uncheckable' }` (everything else — identifier, computed template, +// concatenation, conditional, a bare type reference, or any other expression +// shape — an UNCONDITIONAL violation, independent of which rule eventually +// would have matched the file). Computed/non-static dynamic imports (and +// non-literal import-type expressions) are forbidden in every source file +// covered by at least one generic `RULES` entry; Rule C/Rule D and the other +// already-parser-backed package guards are untouched by this — they already +// classify their own dynamic imports through the real parser and +// #630/#646/#653's package-guard work is not being redone here. import fs from 'node:fs'; import path from 'node:path'; diff --git a/build/lib/check-legacy-owners.mjs b/build/lib/check-legacy-owners.mjs index 68cedb83..c5a7e1ff 100644 --- a/build/lib/check-legacy-owners.mjs +++ b/build/lib/check-legacy-owners.mjs @@ -463,14 +463,40 @@ export function findModuleSpecifiers(source, filename) { // not fall through as if it were absent. `findDynamicImportUsages` is a // SEPARATE entry point (never a modification of the four existing dynamic- // import branches above) that reports a discriminated union for every -// dynamic-import call expression, so nothing is ever silently dropped: a -// `{ kind: 'static', spec }` where the caller's existing resolution logic can -// treat `spec` exactly like an ordinary import, and a `{ kind: 'uncheckable' -// }` that must always become a violation in a generic-guarded file, -// regardless of what its argument might eventually resolve to. A concatenated -// expression such as `import('../' + name)` is `uncheckable` in full — never -// reduced to the quoted `'../'` prefix, since that half alone proves nothing -// about the complete runtime specifier. +// dynamic-import call expression AND every TypeScript inline import-type +// expression (`type T = import('x').Foo`, `typeof import('x')` — its own +// `ImportTypeNode` grammar production, textually indistinguishable from a +// dynamic-import call at the `import(...)` shape `mightContainDynamicImport` +// gates on, but a structurally different AST node a plain +// `is.isCallExpression`-only walk never visits), so nothing is ever silently +// dropped: a `{ kind: 'static', spec }` where the caller's existing +// resolution logic can treat `spec` exactly like an ordinary import, and a +// `{ kind: 'uncheckable' }` that must always become a violation in a +// generic-guarded file, regardless of what its argument might eventually +// resolve to. A concatenated expression such as `import('../' + name)` is +// `uncheckable` in full — never reduced to the quoted `'../'` prefix, since +// that half alone proves nothing about the complete runtime specifier. +// +// Review pass 1 finding: an earlier revision of this function walked ONLY +// `is.isCallExpression` nodes, so `type T = import('../workspace/model.js'). +// Foo` — an `ImportTypeNode`, never a `CallExpression` — contributed nothing +// at all, silently exempting it from the generic `RULES` loop and Rule B even +// though `mightContainDynamicImport`'s gate (a pure `import\b...\(` trivia +// scan, blind to which AST shape follows) correctly let the file through to +// this function. The RETIRED textual `extractSpecifiers` regex this issue +// replaces (`/\bimport\s*\(\s*[`'"]([^`'"]+)[`'"]/g`) could not distinguish a +// call from an inline type expression either — both are just "the word +// `import` then a paren then a quote" to a regex — so it matched and reported +// this form too, meaning the AST-based replacement had strictly LESS coverage +// than the regex it retired for this one shape. Every import-type expression +// is now classified through the exact same discriminated union as a dynamic +// call: its argument is a `LiteralTypeNode` wrapping a string/no-substitution- +// template literal for the ordinary case (`{ kind: 'static', spec }`); any +// other argument shape (e.g. a bare type reference like `import(Bar).Baz`, +// which parses but can never resolve to a real module specifier) is +// `{ kind: 'uncheckable' }`, matching this function's own fail-closed +// contract rather than silently contributing nothing the way +// `findModuleSpecifiers`'s sibling `'import-type'` branch deliberately does. // // `mightContainDynamicImport` is the paired conservative pre-filter (the same // accepted-risk shape as `mightReferencePackage`/`mightReferenceForbiddenRelativeDir` @@ -487,19 +513,25 @@ export function findModuleSpecifiers(source, filename) { // header comment already warns about. /** - * Classify every dynamic `import(...)` call expression in `source` — a real - * TypeScript parse, never a specifier-text regex. Every call expression whose - * callee is the bare `import` keyword contributes exactly one result; there + * Classify every dynamic `import(...)` call expression AND every TypeScript + * inline import-type expression (`type T = import('x').Foo`, `typeof + * import('x')`) in `source` — a real TypeScript parse, never a + * specifier-text regex. Every such node contributes exactly one result; there * is no `if (spec !== null) push(...)` shape here that could silently drop an * unsupported argument (contrast `findModuleSpecifiers` above, whose whole * point is the opposite: silently skip what it cannot resolve, because ITS - * callers have no fail-closed contract to uphold). + * callers have no fail-closed contract to uphold). The two node shapes are + * structurally distinct — a `CallExpression` whose callee is the bare + * `import` keyword token, versus its own `ImportTypeNode` grammar production + * — so both are matched explicitly; classification of the specifier argument + * (a `LiteralTypeNode`'s wrapped literal for the import-type case, in place + * of a call's own first argument) is otherwise identical for both. * * @param {string} source * @param {string} filename repo-relative, forward-slash separated (used only * for the virtual-file basename/grammar selection) * @returns {({kind: 'static', spec: string, pos: number} | {kind: 'uncheckable', pos: number})[]} - * `pos` is the call expression's own start offset (`node.getStart(sourceFile)`) + * `pos` is the matched node's own start offset (`node.getStart(sourceFile)`) * — a stable identity a caller MAY use to de-duplicate the same occurrence * across overlapping guarded-directory rules; it is not a line/column and * is not required in any user-facing diagnostic. @@ -507,30 +539,41 @@ export function findModuleSpecifiers(source, filename) { export function findDynamicImportUsages(source, filename) { return withParsedSource(source, filename, (sourceFile) => { const found = []; + // Shared classification for both node shapes' specifier-bearing argument: + // a plain string/no-substitution-template literal is `static`; every + // other shape — a missing argument, an Identifier, a template literal + // WITH a substitution, a binary concatenation, a call, a conditional, a + // parenthesized/computed expression, a bare type reference (the + // import-type case's own analogous "not actually a literal" shape, + // e.g. `import(Bar).Baz`), or any future shape this list does not name — + // is uncheckable. There is deliberately no partial extraction attempt + // (e.g. reading a template literal's first quasi span): that is precisely + // the class of bug this issue exists to close (`import('../' + name)` + // must never be treated as `'../'`). + const classify = (arg, pos) => { + if ( + arg + && (arg.kind === SyntaxKind.StringLiteral || arg.kind === SyntaxKind.NoSubstitutionTemplateLiteral) + ) { + found.push({ kind: 'static', spec: arg.text, pos }); + } else { + found.push({ kind: 'uncheckable', pos }); + } + }; const walk = (node) => { if ( is.isCallExpression(node) && node.expression && node.expression.kind === SyntaxKind.ImportKeyword ) { - const pos = node.getStart(sourceFile); - const arg = node.arguments[0]; - if ( - arg - && (arg.kind === SyntaxKind.StringLiteral || arg.kind === SyntaxKind.NoSubstitutionTemplateLiteral) - ) { - found.push({ kind: 'static', spec: arg.text, pos }); - } else { - // Every other shape — a missing argument, an Identifier, a - // template literal WITH a substitution, a binary concatenation, a - // call, a conditional, a parenthesized/computed expression, or any - // future shape this list does not name — is uncheckable. There is - // deliberately no partial extraction attempt (e.g. reading a - // template literal's first quasi span): that is precisely the class - // of bug this issue exists to close (`import('../' + name)` must - // never be treated as `'../'`). - found.push({ kind: 'uncheckable', pos }); - } + classify(node.arguments[0], node.getStart(sourceFile)); + } else if (is.isImportTypeNode(node)) { + // `node.argument` is expected to be a `LiteralTypeNode` wrapping the + // actual string/template literal (`.literal`); any other type-node + // shape there (e.g. a `TypeReferenceNode` from `import(Bar).Baz`) + // leaves `.literal` undefined, which `classify` already treats as + // uncheckable — no separate shape check needed here. + classify(node.argument && node.argument.literal, node.getStart(sourceFile)); } node.forEachChild(walk); }; diff --git a/tests/unit/check-boundaries-dynamic-imports.test.js b/tests/unit/check-boundaries-dynamic-imports.test.js index 7e1fedab..e6c89777 100644 --- a/tests/unit/check-boundaries-dynamic-imports.test.js +++ b/tests/unit/check-boundaries-dynamic-imports.test.js @@ -111,6 +111,54 @@ describe('findDynamicImportUsages — uncheckable arguments (never no result, ne }); }); +// Review pass 1 finding: an earlier revision of `findDynamicImportUsages` +// walked ONLY `is.isCallExpression` nodes, so TypeScript's inline import-type +// expression (`type T = import('x').Foo`, `typeof import('x')`) — its own +// `ImportTypeNode` grammar production, never a `CallExpression` — contributed +// nothing at all, even though `mightContainDynamicImport`'s gate (a pure +// textual `import\b...\(` scan, blind to which AST shape follows) correctly +// let the source through to this function. The RETIRED regex this issue +// replaces matched this shape (it can't tell a call from an import-type +// expression either), so the AST-based classifier had strictly LESS coverage +// than what it replaced for this one form until this fix. +describe('findDynamicImportUsages — TypeScript inline import-type expressions', () => { + it("classifies type T = import('../x.js').Foo (single-quoted) as static with the decoded specifier", () => { + const found = findDynamicImportUsages("export type Foo = import('../x.js').Foo;\n", FILE); + expect(found).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('classifies type T = import("../x.js").Foo (double-quoted) as static with the decoded specifier', () => { + const found = findDynamicImportUsages('export type Foo = import("../x.js").Foo;\n', FILE); + expect(found).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('classifies type T = import(`../x.js`).Foo (no-substitution template) as static with the decoded specifier', () => { + const found = findDynamicImportUsages('export type Foo = import(`../x.js`).Foo;\n', FILE); + expect(found).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it("classifies typeof import('../x.js') as static with the decoded specifier", () => { + const found = findDynamicImportUsages("export type Foo = typeof import('../x.js');\n", FILE); + expect(found).toEqual([{ kind: 'static', spec: '../x.js', pos: expect.any(Number) }]); + }); + + it('classifies a bare type-reference argument (import(Bar).Baz) as uncheckable, never thrown', () => { + const found = findDynamicImportUsages('type Bar = string;\nexport type Foo = import(Bar).Baz;\n', FILE); + expect(found).toEqual([{ kind: 'uncheckable', pos: expect.any(Number) }]); + }); + + it('reports one result per import-type expression, alongside an ordinary dynamic import, in source order', () => { + const probe = ` + export type Foo = import('../a.js').Foo; + export async function f(name) { await import(name); } + `; + const found = findDynamicImportUsages(probe, FILE); + expect(found).toHaveLength(2); + expect(found[0]).toEqual({ kind: 'static', spec: '../a.js', pos: expect.any(Number) }); + expect(found[1]).toEqual({ kind: 'uncheckable', pos: expect.any(Number) }); + }); +}); + describe('findDynamicImportUsages — multiple occurrences and clean source', () => { it('reports one result per dynamic-import call expression, in source order', () => { const probe = ` diff --git a/tests/unit/clickhouse-http-package-policy.test.js b/tests/unit/clickhouse-http-package-policy.test.js index f719a51e..e34350a6 100644 --- a/tests/unit/clickhouse-http-package-policy.test.js +++ b/tests/unit/clickhouse-http-package-policy.test.js @@ -565,6 +565,46 @@ describe('Rule A — package source imports no SQL Browser src/** (relative)', ( ]); expect(found.some((line) => line.includes('__boundary_probe_642_legal__'))).toBe(false); }); + + // Review pass 1 finding: an earlier revision of `findDynamicImportUsages` + // never walked `ImportTypeNode`, so this exact form (a structurally + // distinct grammar production, textually identical to a dynamic-import + // call at the `import(...)` shape `mightContainDynamicImport`'s gate + // matches) silently bypassed Rule A entirely — the RETIRED regex this issue + // replaces caught it (it can't distinguish a call from an import-type + // expression either), so the AST-based classifier regressed coverage for + // this one shape until this fix. + it('flags an inline import-type expression reaching into src/application (issue #642 review, sabotage probe, not written to disk)', () => { + const found = relativeViolations(PACKAGE_SRC_DIR, ['src'], [ + ['packages/clickhouse-http/src/__boundary_probe_642_importtype__.ts', + "export type Foo = import('../../../src/application/does-not-exist.js').Foo;\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_importtype__') && line.includes('src/application'))).toBe(true); + }); + + it('flags a typeof import-type expression reaching into src/application (issue #642 review, sabotage probe, not written to disk)', () => { + const found = relativeViolations(PACKAGE_SRC_DIR, ['src'], [ + ['packages/clickhouse-http/src/__boundary_probe_642_importtype_typeof__.ts', + "export type Foo = typeof import('../../../src/application/does-not-exist.js');\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_importtype_typeof__') && line.includes('src/application'))).toBe(true); + }); + + it('flags a bare type-reference import-type argument as uncheckable (issue #642 review, sabotage probe, not written to disk)', () => { + const found = relativeViolations(PACKAGE_SRC_DIR, ['src'], [ + ['packages/clickhouse-http/src/__boundary_probe_642_importtype_bare__.ts', + 'type Bar = string;\nexport type Foo = import(Bar).Baz;\n'], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_importtype_bare__') && line.includes('uncheckable'))).toBe(true); + }); + + it('does not flag a legal relative import-type expression within the package itself (issue #642 review)', () => { + const found = relativeViolations(PACKAGE_SRC_DIR, ['src'], [ + ['packages/clickhouse-http/src/__boundary_probe_642_importtype_legal__.ts', + "export type Foo = import('./client.js').Foo;\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_importtype_legal__'))).toBe(false); + }); }); describe('Rule B — package source has zero bare specifiers (empty allowlist)', () => { @@ -611,6 +651,19 @@ describe('Rule B — package source has zero bare specifiers (empty allowlist)', ]); expect(found.some((line) => line.includes('__boundary_probe_642_bare_relative__'))).toBe(false); }); + + // Review pass 1 finding: an earlier revision of `findDynamicImportUsages` + // never walked `ImportTypeNode`, so a bare-specifier import-type expression + // silently bypassed Rule B too — the same regression as Rule A's own + // import-type sabotage cases above, for the bare-vs-relative half instead + // of the forbidden-directory half. + it('flags a bare import-type expression naming a package specifier (issue #642 review, sabotage probe, not written to disk)', () => { + const found = bareSpecifierViolations(PACKAGE_SRC_DIR, [ + ['packages/clickhouse-http/src/__boundary_probe_642_importtype_bare_pkg__.ts', + "export type Foo = import('left-pad').Foo;\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_importtype_bare_pkg__') && line.includes('left-pad'))).toBe(true); + }); }); // Issue #630 Phase 8 (plan §21, Guard 2) broadens Rule C's forbidden target diff --git a/tests/unit/dashboard-boundaries.test.js b/tests/unit/dashboard-boundaries.test.js index 22fb7a3b..45d12843 100644 --- a/tests/unit/dashboard-boundaries.test.js +++ b/tests/unit/dashboard-boundaries.test.js @@ -237,6 +237,44 @@ describe('dashboard dependency boundaries', () => { expect(found.some((line) => line.includes('__boundary_probe_642_legal__'))).toBe(false); }); + // Review pass 1 finding: an earlier revision of `findDynamicImportUsages` + // never walked `ImportTypeNode`, so this exact form silently bypassed the + // fail-closed pre-pass despite `mightContainDynamicImport`'s gate + // correctly letting it through — the RETIRED regex this issue replaces + // matched it (a call and an import-type expression are textually + // identical at the `import(...)` shape a regex sees). + it('flags an inline import-type expression reaching into src/workspace', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_importtype__.ts', + "export type Foo = import('../workspace/does-not-exist.js').Foo;\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_importtype__') && line.includes('src/workspace'))).toBe(true); + }); + + it('flags a typeof import-type expression reaching into src/workspace', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_importtype_typeof__.ts', + "export type Foo = typeof import('../workspace/does-not-exist.js');\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_importtype_typeof__') && line.includes('src/workspace'))).toBe(true); + }); + + it('rejects a bare type-reference import-type argument as uncheckable', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_importtype_bare__.ts', + 'type Bar = string;\nexport type Foo = import(Bar).Baz;\n'], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_importtype_bare__') && line.includes('uncheckable'))).toBe(true); + }); + + it('accepts a direct import-type expression resolving within an allowed layer', () => { + const found = violations('src/core', FORBIDDEN_CORE, [ + ['src/core/__boundary_probe_642_importtype_legal__.ts', + "export type Foo = import('./format.js').Foo;\n"], + ]); + expect(found.some((line) => line.includes('__boundary_probe_642_importtype_legal__'))).toBe(false); + }); + // Regression coverage for the required static export forms — these must // continue through the existing static fast path, not the new dynamic // parser helper, and must still be rejected when they cross the boundary.