fix(#642): fail closed on computed dynamic imports in check-boundaries RULES

[BorisTyshkevich]

What & why

build/check-boundaries.mjs's generic RULES layering loop (and Rule B) used a regex
(extractSpecifiers) to find dynamic-import specifiers. That regex could only ever
extract an argument that already looked like a complete literal — a computed dynamic
import such as import(\../${name}.js`), import(specifier), or import('../' + name)` either matched nothing (silently exempting the file from every
boundary rule) or, on a concatenated expression, risked being 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.

This adds a new shared, parser-backed classifier
(findDynamicImportUsages/mightContainDynamicImport, in
build/lib/check-legacy-owners.mjs) that every generic-guarded file is now checked
against: a single-quoted, double-quoted, or no-substitution-template-literal argument
remains statically analyzable and flows through the exact 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, with a diagnostic naming the file and explaining the import
cannot be statically checked against the architecture boundary. Rule C/Rule D and the
other already-parser-backed package guards (#646/#653) are untouched — this issue's
scope is only the generic regex-backed RULES path.

An internal review pass found and fixed one residual gap in the new gate's own trivia
handling (bare-CR line terminators and Unicode whitespace/line-separators between
import and its call parens) before this PR was opened — see the two commits.

No product/runtime behavior changed; this hardens the architecture gate's own
soundness, not the policy it enforces.

Closes #642

Checklist

• npm test passes (the per-file coverage gate is non-negotiable)
• Tests added/updated in the same change as the code
• npm run build succeeds (single-file dist/sql.html)
• Layers kept honest: pure logic in src/core/, network in src/net/ (injected fetch), DOM in src/ui/ — n/a, this is a build-tooling change only
• No new runtime dependency
• README / CHANGELOG.md ([Unreleased]) updated
• Reconciled affected tracked work — no roadmap/ADR impact; issue check-boundaries generic RULES do not fail closed on computed dynamic imports #642 itself is closed by this PR

🤖 Generated with Claude Code

https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: 650378dac12120d1618644f65659bee37b377e6e

Findings

1. [Major] Inline TypeScript import() type expressions now bypass the generic RULES loop and Rule B.

   findDynamicImportUsages() only records CallExpression nodes whose callee is ImportKeyword (build/lib/check-legacy-owners.mjs, around lines 507-540). TypeScript's type T = import('pkg').Foo / typeof import('pkg') is instead an ImportTypeNode; the existing findModuleSpecifiers() explicitly has a separate isImportTypeNode branch for exactly this reason. Before this PR, the removed dynamic regex happened to extract the literal from type T = import('../workspace/model.js').Foo; after this PR, extractStaticSpecifiers() does not see it and the new classifier returns no usage, so the generic boundary receives no decision.

   This is a concrete regression for both policies: src/core can use type T = import('../workspace/model.js').Foo without the core→workspace rule seeing it, and packages/clickhouse-http/src can use type T = import('left-pad').Foo without Rule B seeing the bare package specifier. Guard 1 intentionally skips runtime-source bare specifiers because Rule B owns that case. History already documents this grammar distinction in commit eae412d (the Rule-D inline-import-type fix).

   Fix: include ImportTypeNode specifiers in the shared parser-backed generic classification/resolution path (or reuse the existing parser result for kind: 'import-type') and add sabotage coverage in both dashboard-boundaries.test.js and clickhouse-http-package-policy.test.js. Treat a literal import-type specifier as statically analyzable, not as a runtime computed import.

2. [Major] The mandatory same-file audit missed the generic static-import regex's existing fail-open forms.

   extractStaticSpecifiers() still uses \bimport\s+[\w*{}\s,]+... / equivalent export regexes. A valid import/*c*/ { x } from '../workspace/x.js' is invisible, while an escaped specifier such as import { x } from '../worksp\x61ce/x.js' is captured as raw escaped text and therefore resolves to the wrong lexical path rather than src/workspace. Both cases leave the generic boundary decision fail-open. The surrounding file already documents this exact comment/escaped-literal failure mode as the reason Rule C/D moved to the parser.

   The attached review contract explicitly asks this pass to check every other regex/prefilter in check-boundaries.mjs for the same unsound pattern. This is the same root class, not an unrelated cleanup.

   Fix: route the generic static module-specifier forms through the existing parser-backed module-specifier helper (preferably with a conservative gate/caching strategy if timing matters), rather than widening another regex; add comment-trivia and escaped-specifier sabotage cases.

3. [Major] The mandatory prefilter audit also leaves a raw source.includes(name) gate in front of real-parser Guards 3/4 and the Phase-7 retired-API guard.

   mightReferenceRetiredTopLevelApi() still returns names.some(name => source.includes(name)), and check-boundaries.mjs uses it to decide whether to invoke findTransportSurfaceOwnershipViolations() and findRetiredTopLevelApiViolations(). A valid escaped identifier such as export const run\u0051uery = ... binds the exact identifier runQuery, but contains no raw runQuery substring, so the real parser is skipped. The repository's own ship-review guidance specifically says that, when touching an architecture parser prefilter, every other bare source.includes(name) prefilter in the file must be checked for this class of escape.

   Fix: make this shared prefilter escape-aware (at minimum route any source containing a backslash through the parser, matching the established conservative pattern) and add an escaped-identifier sabotage case for Phase 7 plus one Guards 3/4 name.

Validation

The canonical CI workflow for this exact head is green. A direct local clone/fetch was not possible in the review sandbox because github.com DNS resolution is unavailable, so I could not rerun the repository's Vitest files locally. I did run focused standalone probes against the exact current regex/prefilter expressions: the current static scanner misses the comment-trivia import, the pre-PR dynamic arm matches the inline ImportTypeNode spelling that the new classifier drops, and the raw-name prefilter returns false for a valid Unicode-escaped runQuery identifier.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: 650378dac12120d1618644f65659bee37b377e6e
Reviewed head: a2318aa2612ab65f1f059649ad51506744b667d4

Reassessment of pass 1

1. RESOLVED — inline TypeScript import-type expressions.

   The new commit explicitly adds an is.isImportTypeNode(node) branch to findDynamicImportUsages(), feeding the wrapped literal through the same {kind: 'static'|'uncheckable'} classifier as runtime import(...). The updated unit and policy-mirror suites cover single/double/no-substitution-template import types, typeof import(...), uncheckable non-literal forms, forbidden relative targets, legal relative targets, and Rule B bare package specifiers. I do not see a residual form-specific bypass in this fix.

2. [Major, UNRESOLVED] The generic static-import scanner still fails open on valid static syntax.

   build/check-boundaries.mjs still contains the same three regexes in SPECIFIER_PATTERNS, including \bimport\s+[\w*{}\s,]+... and the analogous export/bare-import forms. This means the pass-1 examples remain reproducible at the new head: import/*c*/ { x } from '../workspace/x.js' produces no specifier at all, while import { x } from '../worksp\x61ce/x.js' yields the raw escaped text instead of the decoded ../workspace/x.js, so resolveRelative() compares the wrong path. The file itself documents this exact comment/escaped-literal failure mode as the reason Rule C moved to findModuleSpecifiers().

   This is directly within the unit's mandatory same-file audit question, not unrelated cleanup. Fix: stop using the static regex as the authoritative generic RULES/Rule B module-specifier extractor; reuse the existing real-parser module-specifier helper with a conservative cache/prefilter if timing requires it. Add sabotage cases for comment trivia and escaped specifiers across static import/re-export/bare-import forms.

3. [Major, UNRESOLVED] The raw-name parser prefilter still skips Unicode-escaped identifiers.

   mightReferenceRetiredTopLevelApi() is still exactly names.some((name) => source.includes(name)), and production still uses it to gate both Guards 3/4 and the Phase-7 retired-top-level-API parser walks. A valid escaped identifier such as const run\u0051uery = 7 does not contain the raw substring runQuery, so the parser is never called even though the language resolves the binding to the exact banned identifier. A focused executable probe against the current expression confirms source.includes('runQuery') === false while evaluating the declaration and reading runQuery succeeds.

   This is also the exact prefilter class the repository's architecture-review guidance says to audit whenever one such prefilter is touched. Fix: make this shared gate conservative for escaped identifiers (at minimum, any backslash should route the source through the parser, matching the established mightReferencePackage pattern) and add sabotage tests for one Phase-7 name and one Guard-3/4 name.

Complete updated-PR check

I re-read the complete six-file PR diff, the one-commit delta from the prior reviewed SHA, the production generic-RULES/Rule-B flow, the shared parser helpers, and the updated policy mirrors. The unique-file pre-pass still deduplicates through guardedFileSources, and Rule B still consumes the cached static classifier results rather than reclassifying. I found no additional material regression beyond the two unresolved pass-1 findings above.

Validation

Canonical CI for a2318aa2612ab65f1f059649ad51506744b667d4 is green. A local clone/fetch remains impossible in this review sandbox because github.com DNS resolution fails, so I could not rerun repository Vitest directly. I did run focused executable probes against the exact current static-regex and raw-name-prefilter expressions; both unresolved bypasses reproduce.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed anchor: 650378dac12120d1618644f65659bee37b377e6e
Reviewed head: a2318aa2612ab65f1f059649ad51506744b667d4

The PR head has not moved since pass 2; relative to the requested anchor it is one commit ahead.

Reassessment of earlier findings

1. RESOLVED — TypeScript inline import-type expressions.

   The post-anchor commit explicitly handles ImportTypeNode alongside runtime dynamic-import CallExpressions. Both node shapes feed the same classifier: only StringLiteral and NoSubstitutionTemplateLiteral become {kind:'static'}; every other argument shape becomes {kind:'uncheckable'}. The focused tests and both policy mirrors now cover direct/typeof import types, legal/forbidden relative targets, a Rule-B bare package specifier, and a non-literal import-type argument.

2. NOT A #642 BLOCKER — the pre-existing static-import regex limitations raised in pass 2.

   I rechecked the pass-3 rebuttal against the merge-base/current diff. The three static-only regex arms are pre-existing behavior; #642 removes only the fourth dynamic-import regex arm and renames the helper to extractStaticSpecifiers. The comment-trivia/escaped-static-specifier limitation is real, but this PR does not introduce or broaden it, and the delivery contract explicitly keeps #642 scoped to dynamic import(...) fail-closed classification. I am withdrawing that item as a defect of this unit.

3. NOT A #642 BLOCKER — the pre-existing mightReferenceRetiredTopLevelApi() escape policy raised in pass 2.

   This helper is unchanged by the PR and its narrower escaped-identifier threat model predates #642. I found no new call site or changed behavior in this diff that makes the pre-existing policy a regression of this unit, so I am not carrying it forward as a pass-3 finding.

Focused pass-3 audit

• Argument AST shapes: the classifier cannot accidentally treat another expression shape as static: the static branch is an exact two-kind check (StringLiteral / NoSubstitutionTemplateLiteral), and every other runtime-call argument falls into uncheckable. The added ImportTypeNode branch applies the same closed classification to the wrapped type literal.
• Unique-file pre-pass: guardedFileSources is keyed by absolute path before classification, so overlapping directories such as src/dashboard and src/dashboard/application cause one parser classification per unique file. The per-rule loop only consumes the cache.
• Rule B reuse: package source is already in generic Rule A, and Rule B reads guardedFileSources plus guardedFileDynamicImports; it does not re-run the dynamic classifier.
• Gate: I found no additional legal-trivia false negative after the 650378d CR/Unicode fix.
• Additional shape probe: I checked JSDoc import(...) type text as a potential parser-walk omission. The repository's TypeScript configuration states all hand-written modules are strict .ts; allowJs is retained only for generated artifacts and checkJs is false. That does not establish a supported-source #642 regression.

Complete updated-PR check

I re-read the complete six-file PR diff and the full 650378d... -> a2318aa... delta. I found no new material regression within the delivery contract after the import-type fix.

Canonical CI for a2318aa2612ab65f1f059649ad51506744b667d4 is green. A direct clone/fetch remains unavailable in this sandbox because github.com DNS resolution fails, so I could not rerun the repository's Vitest suite locally.

VERDICT: SHIP