Repository navigation
check-boundaries generic RULES do not fail closed on computed dynamic imports #642
Description
Status
The original backtick/no-substitution-template bypass reported here was fixed in #646:
import(`@altinity/clickhouse-http`)is now recognized, and the package-boundary rules that needed stronger syntax handling were moved to the real TypeScript-parser-backed helpers during #646/#653.
The remaining problem is narrower: the generic regex-backed RULES in build/check-boundaries.mjs still do not fail closed on dynamic imports whose argument is not a statically provable string literal.
Problem
extractSpecifiers() only returns module specifiers it can recognize textually. That is sufficient for ordinary static imports/exports and for quoted or no-substitution-template dynamic imports, but it silently skips computed dynamic imports such as:
import(specifier)
import(`../${name}.js`)
import(prefix + '/module.js')For a source tree guarded only by the generic RULES loop, a computed dynamic import therefore receives no boundary decision at all. The check does not prove that the import is allowed; it simply fails to see a specifier.
This is no longer the broad package-boundary hole described in the original filing: later #630 phases moved Rule C/Rule D and the final package guards to the real parser. The remaining scope is the generic internal-layer rules that still intentionally use extractSpecifiers().
Why it matters
These guards are intended to enforce source-level dependency direction. A rule that silently ignores an import form it cannot classify is fail-open: a future dynamic import can cross a forbidden layer without check:arch noticing.
There is no known production use of computed dynamic imports under the currently guarded trees, so this is architecture-hardening rather than a live product regression.
Intended fix
For directories covered by the generic RULES, make dynamic-import handling fail closed:
- allow a dynamic import only when its argument is statically provable as a string-like module specifier supported by the checker;
- continue to accept single-quoted, double-quoted, and no-substitution-template literals;
- reject computed/non-static
import(...)expressions in guarded source instead of silently skipping them; - preserve the existing fast path for ordinary static import/export forms unless moving the generic rules to the shared real-parser helper is simpler and measurably acceptable.
The diagnostic should identify the file and explain that the dynamic import cannot be statically checked against the architecture boundary.
Tests
Add sabotage coverage for at least:
- quoted dynamic import crossing a forbidden boundary;
- no-substitution-template dynamic import crossing a forbidden boundary;
- computed template dynamic import, e.g.
import(`../${name}.js`); - identifier argument, e.g.
import(specifier); - concatenated argument, e.g.
import('../' + name); - existing
export * from, named re-export, and namespace re-export coverage remains intact.
The production checker and its policy mirror should prove the same fail-closed behavior.
Acceptance criteria
- A guarded source file cannot use a computed/non-static dynamic import without
check:archfailing. - Single-quoted, double-quoted, and no-substitution-template dynamic imports remain analyzable.
- A statically analyzable dynamic import crossing a forbidden
RULESboundary is rejected. - Existing legal imports remain accepted.
- Sabotage tests cover computed-template, identifier, and concatenated dynamic-import arguments.
- The change does not reintroduce a second unsound hand-rolled parser where the shared TypeScript-parser helper is the more appropriate mechanism.
History
Originally filed during review of #641 after finding that the then-current dynamic-import regex matched only single- and double-quoted arguments. That exact bypass was fixed in #646. This issue now tracks only the remaining fail-open behavior for non-static dynamic imports in the generic regex-backed architecture rules.
Activity
[-]check-boundaries.mjs's dynamic-import scanner misses backtick no-substitution template literals[/-][+]check-boundaries generic RULES do not fail closed on computed dynamic imports[/+]fix(#642): fail closed on computed dynamic imports in check-boundarie…
fix(#642): close CR/Unicode-trivia gap in mightContainDynamicImport
BorisTyshkevich commentedon Aug 10, 2026
Ship log — #642
Handoff
build/lib/check-legacy-owners.mjsgains a shared parser-backedfindDynamicImportUsages/mightContainDynamicImportpair;build/check-boundaries.mjs'sgeneric
RULESloop and Rule B now classify every dynamicimport(...)in aguarded file via that pair instead of the old dynamic-import regex arm
(removed from
extractSpecifiers, renamedextractStaticSpecifiers).Uncheckable arguments (identifier, computed template, concatenation,
conditional, TypeScript inline import-type non-literal, or otherwise) are
unconditional violations; statically analyzable arguments (single/double-
quoted, no-substitution-template, including inline
type T = import('x').Fooforms) flow through the existing relative-resolution/forbidden-prefix logic
unchanged. Rule C/Rule D and other already-parser-backed package guards
(feat(#630): extract ClickHouse SQL quoting and type-expression grammar (phase 5) #646/feat(#630): make @altinity/clickhouse-http standalone-buildable, retire client-web vendor spike (phase 8) #653) are untouched.
of every other regex/prefilter in
check-boundaries.mjsfound no otherinstance of the fail-open pattern this issue targets (two ChatGPT findings
proposing to widen scope to
extractStaticSpecifiers's static-only regexand to
mightReferenceRetiredTopLevelApi's identifier-only threat modelwere independently re-verified and rejected across two passes — both are
real, pre-existing, already-documented, out-of-scope design boundaries
from earlier closed issues, not defects this PR introduced).
claimed, none deferred, per the approved plan.
inbox) — a real chatgpt-review tooling bugfound while shipping this unit:
extractReportedMetadata's SHA-extractionregexes don't tolerate Markdown bold formatting around a backtick-wrapped
SHA (
**\`**`), silently falling back to "first 40-hex string in thetext" and reporting the WRONG (earlier/previous) SHA as the reviewed head
on multi-pass PR reviews. Confirmed by manually reading the actual GitHub
PR review comments, which correctly named the current head both times —
this is purely an extraction bug, not a review defect, and did not affect
the correctness of this unit's own certification (independently manually
verified before merging).
conversation https://chatgpt.com/c/6a79c43b-f11c-83eb-93b7-f4378d063f32.
a2318aa2612ab65f1f059649ad51506744b667d4after 3 code-review passes — pass 1 found and fixed one real regression
(TypeScript inline
import(...)type expressions bypassing the newclassifier,
import('../workspace/model.js')-shaped but as anImportTypeNoderather than aCallExpression); passes 2 and 3 bothcorrectly re-rejected the same two out-of-scope findings on adversarial
re-verification; pass 3 verdict SHIP, no accepted findings. Reviewed-SHA
match confirmed manually (see chatgpt-review: extractReportedMetadata misextracts reviewed SHA when Markdown-bolded #669). Merged as
358924a.fix(#642): address review pass 1 findings
Merge pull request #668 from Altinity/fix/642-dynamic-import-fail-closed
fix(#643): replace regex comment stripping with parser-backed archite…