MemberJunction / MemberJunction/MJ
Audit MJ for the #3677 bug class: dynamic imports of runtime-configured or undeclared specifiers that break under pnpm
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
PR #3717 fixed the two instances of this class that block Open Apps (server-bootstrap registration, open-app-engine migrations), but an adversarial audit of that PR found the **class persists elsewhere**. This issue requests a **full repo-wide audit**, not just fixes for the sites below.
## The bug class
A dynamic `import()` whose specifier is runtime-configured (or simply undeclared by the importing package) resolves from the **importing package**, which under pnpm's strict layout only sees what it declares. npm's hoisting masked every instance of this; pnpm surfaces them one by one as packages are exercised on 6.x. Failure modes: hard `ERR_MODULE_NOT_FOUND` at runtime, or — worse — silent wrong-copy loading when a stale registry copy shadows the intended one.
## Known instances (verified on next @ PR #3717's tip)
| Site | Specifier source | Risk |
|---|---|---|
| `packages/AI/Engine/src/AIEngine.ts:1535` | DB-configured `DriverImportPath` (`await import(driverModuleName)`) — AI Engine declares no driver packages | **High** — the most likely next real-world hit; any AI driver configured via metadata fails to load under pnpm exactly like #3677 |
| `packages/CLICore/src/plugin-registry.ts:98` | bare-specifier fallback for plugin loading | Medium |
| `packages/TestingFramework/CLI/src/utils/check-module-loader.ts:63` | bare-specifier fallback | Low (tooling) |
| `packages/Actions/Runtime/harness/run-demos.ts:50` | `await import(pkg)` over a package list | Low (demo harness) |
## Requested audit scope
1. **Every non-literal dynamic import** across `packages/`: `grep -rnE "await import\((?!['\"])" --include='*.ts'` (plus template-literal and variable-specifier forms), classifying each as: (a) declared by the importer → fine, (b) runtime-configured names → needs host-anchored resolution, (c) undeclared static contract → needs a declaration.
2. **Undeclared static imports** masked by hoisting — the existing dependency-check CI catches some; confirm its coverage includes every `"type": "module"` package and dev-only entry points.
3. Decide the shared mechanism: PR #3717 established two patterns — declare (optionalDependencies for optional runtime deps) and **host-anchored resolution** (`server-bootstrap/src/host-import.ts`: bare import first, then `createRequire` retries anchored at the config that named the package → cwd → process entrypoint, with resolution/evaluation separated so a found module's own failure is never masked). If three or more packages need the latter, it likely deserves a shared utility package rather than more keep-in-sync copies (there are already two: `host-import.ts` and `migration-runner.ts`'s predicate).
## Acceptance
- An inventory of every dynamic-import site with its classification (including the ones judged fine, so coverage is explicit)
- Fixes or tracked follow-ups for every (b)/(c) site
- A CI guard so new sites of this class fail review instead of shipping (the existing dependency-check bot catches static undeclared imports; non-literal dynamic imports currently pass silently)
Found during the audit of PR #3717 (fixes #3677); reported from MJDev.
Contributor guide
Research direction
Start with the listed dynamic-import sites, especially packages/AI/Engine/src/AIEngine.ts, packages/CLICore/src/plugin-registry.ts, packages/TestingFramework/CLI/src/utils/check-module-loader.ts, and packages/Actions/Runtime/harness/run-demos.ts. Run the requested grep across packages/ and review server-bootstrap/src/host-import.ts plus the dependency-check CI coverage. Done means a complete classified inventory, fixes or follow-ups for every problematic site, and a CI guard for new non-literal imports.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- node.js, typescript
- Domain
- build-system, tooling
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100