Provide a better-sqlite3 module backed by bun:sqlite - #36712
Code review found 2 important issues
Found 5 candidates, confirmed 9. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 7 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/js/thirdparty/better-sqlite3.ts:246 |
Named parameters silently bind NULL: shim opens bun:sqlite without strict mode |
| 🟡 Nit | src/codegen/internal-module-registry-scanner.ts:18 |
bun/* module at index 0 unresolvable due to truthy check on registry lookup |
| 🟡 Nit | src/js/thirdparty/better-sqlite3.ts:303-305 |
transaction().database leaks the internal bun:sqlite Database instead of the shim wrapper |
| 🟡 Nit | src/js/thirdparty/better-sqlite3.ts:248-252 |
Empty catch{} swallows PRAGMA busy_timeout failure |
| 🟡 Nit | src/js/thirdparty/better-sqlite3.ts:224-233 |
verbose option is validated but never invoked |
Annotations
Check failure on line 246 in src/js/thirdparty/better-sqlite3.ts
claude / Claude Code Review
Named parameters silently bind NULL: shim opens bun:sqlite without strict mode
The underlying `BunDatabase` is opened without `strict: true`, so bun:sqlite never strips the leading `@`/`:`/`$` from named-parameter keys — better-sqlite3's documented `stmt.run({ name: 'x' })` for `VALUES (@name)` silently binds NULL instead of `'x'` (and doesn't throw, since `throwOnMissing` follows the same flag). Pass `strict: true` in both branches (buffer and filename) so named parameters match better-sqlite3 semantics.
Check warning on line 18 in src/codegen/internal-module-registry-scanner.ts
claude / Claude Code Review
bun/* module at index 0 unresolvable due to truthy check on registry lookup
Now that `bun/*` files are actually registered, `bun/ffi.ts` sorts to `moduleList[0]` and `internalRegistry` gets `"bun:ffi" → 0` — but `requireTransformer` (line 67) checks `if (directMatch)`, so index 0 falls through to filesystem resolution and would fail. Nothing currently `require("bun:ffi")`s from a builtin so this is latent, but since you're touching this file anyway, changing line 67 to `if (directMatch !== undefined)` would make the fix cover all three `bun:*` modules instead of just th
Check warning on line 305 in src/js/thirdparty/better-sqlite3.ts
claude / Claude Code Review
transaction().database leaks the internal bun:sqlite Database instead of the shim wrapper
`db.transaction(fn).database` (and `.deferred.database` / `.immediate.database` / `.exclusive.database`) returns the internal `bun:sqlite` `Database`, not the better-sqlite3 shim instance the user constructed — bun:sqlite's `transaction()` sets `database: { value: this }` and the shim passes the result through unchanged. In real better-sqlite3 `tx.database === db`; here identity fails and callers navigating back via `tx.database` get an object with a different API surface (no chainable `.raw()`/
Check warning on line 252 in src/js/thirdparty/better-sqlite3.ts
claude / Claude Code Review
Empty catch{} swallows PRAGMA busy_timeout failure
This bare `catch {}` is exactly the pattern REVIEW.md's error-handling section rejects ("convert diagnosable errors into silent corruption"). On a freshly-opened handle with an already-validated non-negative integer ≤ 0x7fffffff, `PRAGMA busy_timeout = N` cannot fail — real better-sqlite3 calls `sqlite3_busy_timeout()` directly, which always returns `SQLITE_OK` — so the try/catch is dead defensive code. Either remove it and let a hypothetical failure surface at open time, or add a one-line comme
Check warning on line 233 in src/js/thirdparty/better-sqlite3.ts
claude / Claude Code Review
verbose option is validated but never invoked
The `verbose` option is read and type-validated (throws `TypeError` if not a function), but the callback is never invoked anywhere — in real better-sqlite3 it's called with the SQL text of every executed statement (commonly `{ verbose: console.log }`). Either wire it up (call `verbose(source)` in `prepare` / `exec` / `pragma`) or add an "accepted and ignored" comment like the adjacent `nativeBinding` line has — REVIEW.md flags parsed-but-never-read state as a red flag.