Fix dynamic import() resolving before TLA module finishes evaluating #29224
+204
−0
Claude / Claude Code Review
completed
Apr 12, 2026 in 11m 31s
Code review found 1 important issue
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/js/builtins/ModuleLoaderOverrides.ts:73 |
typeof .then check instead of $isPromise in TLA evaluation guard |
| 🟡 Nit | test/regression/issue/29221.test.ts:32-53 |
Test spawns with path concatenation and combines stdout/exitCode assertions |
Annotations
Check failure on line 73 in src/js/builtins/ModuleLoaderOverrides.ts
claude / Claude Code Review
typeof .then check instead of $isPromise in TLA evaluation guard
The Promise detection at `ModuleLoaderOverrides.ts:73` uses a duck-typing `typeof .then === "function"` check instead of the tamper-proof `$isPromise()` intrinsic used everywhere else in Bun's builtins — if user code runs `delete Promise.prototype.then` before evaluation, `entry.evaluatingPromise` is never set and all concurrent `import()` callers bypass the TLA wait entirely, defeating this PR's fix. Replace `evalResult && typeof (evalResult as any).then === "function"` with `evalResult && $isP
Check warning on line 53 in test/regression/issue/29221.test.ts
claude / Claude Code Review
Test spawns with path concatenation and combines stdout/exitCode assertions
Two minor test convention violations in `29221.test.ts` per CLAUDE.md guidelines: (1) line 33 spawns with `String(dir) + "/entry.mjs"` instead of the portable `cmd: [bunExe(), "entry.mjs"], cwd: String(dir)` pattern — the hardcoded "/" separator is fragile on Windows; (2) lines 47–53 combine `stdout` and `exitCode` into a single `toEqual` call instead of two separate assertions (stdout first), which gives less diagnostic clarity when the test fails.
Loading