refactor: add SQLite adapter to decouple from bun:sqlite#970
Conversation
|
Codecov Results 📊✅ 6969 passed | Total: 6969 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ❌ Patch coverage is 45.24%. Project has 14253 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 77.06% 76.91% -0.15%
==========================================
Files 320 321 +1
Lines 61606 61739 +133
Branches 0 0 —
==========================================
+ Hits 47477 47486 +9
- Misses 14129 14253 +124
- Partials 0 0 —Generated by Codecov Action |
Introduce src/lib/db/sqlite.ts — a runtime-detecting adapter that provides a unified Database API for SQLite access. Under Bun it re-exports bun:sqlite directly (zero overhead). Under Node.js it wraps node:sqlite's DatabaseSync with the same .query()/.exec()/ .close()/.transaction() interface. This eliminates all direct bun:sqlite imports from src/ and test/, making the DB layer portable across runtimes without changing any call sites. Changes: - New: src/lib/db/sqlite.ts (adapter with runtime detection) - Updated: db/index.ts, schema.ts, migration.ts, utils.ts imports - Updated: 3 test files to import from adapter - Added: MIGRATION-PLAN.md documenting remaining migration steps
c0b3adf to
972b864
Compare
|
|
||
| exec(sql: string): void { | ||
| this.db.exec(sql); |
There was a problem hiding this comment.
isSchemaError and isReadonlyError never trigger under Node.js, silently disabling auto-repair
The error-classification functions in schema.ts (lines 595, 615) gate on error.name === 'SQLiteError', which is the bun:sqlite error class name. node:sqlite throws plain Error instances with error.code === 'ERR_SQLITE_ERROR' and error.name === 'Error', so both functions always return false on Node.js — disabling the entire auto-repair system and read-only detection. Add a check for error.code === 'ERR_SQLITE_ERROR' alongside the existing name check in schema.ts.
Verification
Traced the full path: Database.exec() (sqlite.ts:84) and StatementWrapper.run() (sqlite.ts:49) are now the only error-throwing surfaces for callers. Under node:sqlite, all SQLite-level errors are thrown as standard Error instances with err.code === 'ERR_SQLITE_ERROR' (per Node.js 22 docs and source). Under bun:sqlite they are SQLiteError instances. schema.ts:595 checks error instanceof Error && error.name === 'SQLiteError' — this is the bun:sqlite class name, and will never be true for node:sqlite errors. Consequently tryRepairAndRetry(), isSchemaError(), and isReadonlyError() all silently return false on Node.js, meaning: (a) schema auto-repair never runs, (b) read-only database errors are never surfaced correctly. No test exists that exercises these functions with a real node:sqlite error object.
Identified by Warden find-bugs · 9N4-5DY
| const result = fn(); | ||
| this.db.exec("COMMIT"); | ||
| return result; | ||
| } catch (error) { | ||
| this.db.exec("ROLLBACK"); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
ROLLBACK exec failure silently discards the original transaction error
If fn() throws and this.db.exec('ROLLBACK') also throws (e.g., connection is closed or already in an error state), the ROLLBACK exception propagates and the original error is permanently lost, making failures very hard to diagnose. Use a try/catch around the ROLLBACK call and re-throw the original error regardless.
Verification
Lines 107-113 of the manual node:sqlite transaction path: the catch (error) block calls this.db.exec('ROLLBACK') directly. If that exec call itself throws (SQLite error on an already-aborted transaction, or a closed database), the new exception replaces error before throw error on line 112 executes. The bun:sqlite path (native transaction()) does not have this problem. No test exists that covers this error-within-error path. Fix: wrap ROLLBACK in its own try/catch and always re-throw the captured error.
Identified by Warden find-bugs · HZ6-E88
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 972b864. Configure here.
|
|
||
| query(sql: string): StatementWrapper { | ||
| // node:sqlite uses .prepare(), bun:sqlite uses .query() | ||
| const prepFn = this.db.prepare ?? this.db.query; |
There was a problem hiding this comment.
Nullish coalescing operands reversed in query method
Medium Severity
The expression this.db.prepare ?? this.db.query always resolves to .prepare() because both node:sqlite's DatabaseSync and bun:sqlite's Database define a .prepare() method — it's never nullish. The ?? this.db.query fallback is dead code. The comment says "bun:sqlite uses .query()" but .query() is never actually called. Under bun:sqlite, .query() caches compiled statement bytecode while .prepare() does not, so this is a performance regression. More critically, bun:sqlite has a known bug where .prepare() ignores single binding arguments (passing NULL instead), which could cause query failures. The operands likely need to be reversed to this.db.query ?? this.db.prepare.
Reviewed by Cursor Bugbot for commit 972b864. Configure here.


Summary
Second step of the Bun → Node.js migration (follows #967). Introduces a SQLite adapter layer that decouples the codebase from direct
bun:sqliteimports, making the database layer portable across runtimes.Changes
src/lib/db/sqlite.ts— SQLite adapter usingnode:sqlite(Node 22+)bun:sqlitewhennode:sqliteis unavailable (during Bun test runner transition only — fallback will be removed once tests migrate to Vitest)node:sqliteuses.prepare()whilebun:sqliteuses.query();node:sqlitelacks.transaction()so a manual BEGIN/COMMIT/ROLLBACK wrapper is usedDatabaseclass andSQLQueryBindingstypedb/index.ts,schema.ts,migration.ts,utils.tsfix.test.ts,telemetry.test.ts,schema.test.tsMIGRATION-PLAN.mddocumenting remaining migration stepsDesign
Call sites continue using
.query(sql).get()/.all()/.run()anddb.transaction()exactly as before — no migration churn needed.Zero
bun:sqliteimports remain insrc/ortest/.