fix: replace better-sqlite3 with bun:sqlite for schema cache and SQLite driver#323
fix: replace better-sqlite3 with bun:sqlite for schema cache and SQLite driver#323anandgupta42 merged 3 commits intomainfrom
Conversation
…SQLite driver
`better-sqlite3` is a native addon that doesn't work on Bun ("not yet supported").
This broke `schema_index`, `schema_search`, `schema_cache_status`, and the SQLite
driver for all users on the released CLI binary.
Switch to `bun:sqlite` which is built into the Bun runtime — zero-install, no
native compilation, same synchronous API. The storage layer (`db.ts`) already
uses this pattern successfully.
Changes:
- `schema/cache.ts`: direct `bun:sqlite` import, sync `create()`/`createInMemory()`
- `drivers/sqlite.ts`: `bun:sqlite` import, fix PRAGMA+LIMIT syntax error
- Remove `better-sqlite3` from optional deps, peer deps, build externals, types
- Update driver docs and E2E tests (SQLite tests no longer need skip guards)
Closes #314
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughReplaces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
34 new tests covering: - Upgrade path from `better-sqlite3` (legacy DB files opened by `bun:sqlite`) - Corrupted/truncated/zero-byte database files - SQL injection resistance via search queries (7 injection vectors + null bytes) - Unicode and special character identifiers in schema/table/column names - Large dataset stress (1000 tables x 10 columns) - Re-indexing data replacement and multi-warehouse isolation - Connector failure modes (listSchemas/listTables/describeTable errors) - Search edge cases (empty, stop words, long query, case insensitivity, FQN) - `listColumns` with limits and unknown warehouses - File-based cache persistence across close/reopen - Singleton lifecycle (getCache/resetCache) - PRAGMA LIMIT syntax fix verification
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opencode/src/altimate/native/schema/cache.ts (1)
117-125: Consider adding a guard against using a closed database.Unlike
sqlite.tswhich guards againstdbbeingnull, theSchemaCacheclass doesn't have similar protection. Afterclose()is called (lines 368-374), thedbreference becomes invalid but isn't nullified. Subsequent method calls would throw opaque SQLite errors rather than a clear "cache closed" message.♻️ Optional: Add closed-state guard
export class SchemaCache { private db: Database private dbPath: string + private closed = false private constructor(db: Database, dbPath: string) { this.db = db this.dbPath = dbPath } + + private ensureOpen(): void { + if (this.closed) throw new Error("SchemaCache has been closed") + }Then call
this.ensureOpen()at the start ofindexWarehouse,search,cacheStatus, andlistColumns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/native/schema/cache.ts` around lines 117 - 125, SchemaCache lacks a closed-state guard so after close() the db stays set and subsequent calls throw opaque SQLite errors; add an internal ensureOpen() method on the SchemaCache class that throws a clear "SchemaCache is closed" error if this.db is null/closed, call this.ensureOpen() at the start of public methods indexWarehouse, search, cacheStatus, and listColumns, and update close() to null out or mark the db field (e.g., set this.db = null or this._closed = true) so ensureOpen() can detect the closed state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/altimate/drivers-e2e.test.ts`:
- Around line 550-555: The test "handles invalid SQL gracefully" incorrectly
uses expect(() => connector.execute(...)).toThrow() for the async method
connector.execute; change the assertion to handle the returned Promise by using
either await expect(connector.execute("INVALID SQL
STATEMENT")).rejects.toThrow() or return expect(connector.execute("INVALID SQL
STATEMENT")).rejects.toThrow() so the rejection is asserted correctly.
---
Nitpick comments:
In `@packages/opencode/src/altimate/native/schema/cache.ts`:
- Around line 117-125: SchemaCache lacks a closed-state guard so after close()
the db stays set and subsequent calls throw opaque SQLite errors; add an
internal ensureOpen() method on the SchemaCache class that throws a clear
"SchemaCache is closed" error if this.db is null/closed, call this.ensureOpen()
at the start of public methods indexWarehouse, search, cacheStatus, and
listColumns, and update close() to null out or mark the db field (e.g., set
this.db = null or this._closed = true) so ensureOpen() can detect the closed
state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 53cfda2d-d4d7-431a-99c9-be1d9cc82e53
📒 Files selected for processing (8)
docs/docs/drivers.mdpackage.jsonpackages/drivers/package.jsonpackages/drivers/src/sqlite.tspackages/opencode/script/build.tspackages/opencode/script/publish.tspackages/opencode/src/altimate/native/schema/cache.tspackages/opencode/test/altimate/drivers-e2e.test.ts
💤 Files with no reviewable changes (2)
- package.json
- packages/opencode/script/publish.ts
| test( | ||
| "handles invalid SQL gracefully", | ||
| async () => { | ||
| expect(() => connector.execute("INVALID SQL STATEMENT")).toThrow() | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Incorrect assertion: expect(...).toThrow() should be expect(...).rejects.toThrow() for async function.
connector.execute() is an async function that returns a Promise. Using expect(...).toThrow() synchronously will not catch the rejection—it will likely pass incorrectly or throw unexpectedly. The test should use rejects.toThrow() consistent with line 569.
🐛 Proposed fix
test(
"handles invalid SQL gracefully",
async () => {
- expect(() => connector.execute("INVALID SQL STATEMENT")).toThrow()
+ await expect(connector.execute("INVALID SQL STATEMENT")).rejects.toThrow()
},
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test( | |
| "handles invalid SQL gracefully", | |
| async () => { | |
| expect(() => connector.execute("INVALID SQL STATEMENT")).toThrow() | |
| }, | |
| ) | |
| test( | |
| "handles invalid SQL gracefully", | |
| async () => { | |
| await expect(connector.execute("INVALID SQL STATEMENT")).rejects.toThrow() | |
| }, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/drivers-e2e.test.ts` around lines 550 - 555,
The test "handles invalid SQL gracefully" incorrectly uses expect(() =>
connector.execute(...)).toThrow() for the async method connector.execute; change
the assertion to handle the returned Promise by using either await
expect(connector.execute("INVALID SQL STATEMENT")).rejects.toThrow() or return
expect(connector.execute("INVALID SQL STATEMENT")).rejects.toThrow() so the
rejection is asserted correctly.
✅ Tests — All PassedTypeScript — passedcc @anandgupta42 |
…g, tests Fixes from 6-model consensus code review: 1. **CRITICAL** — SQLite driver `readonly` + `create: true` bug: gate `create` and WAL pragma on `!readonly` so readonly connections don't silently open read-write or crash on PRAGMA WAL. 2. **MAJOR** — Wrap `indexWarehouse` inserts in `db.transaction()` per-table to avoid per-statement disk fsyncs (~200x slowdown for large warehouses). 3. **MAJOR** — Fix no-op parent directory test (was creating dir before testing). Add 3 readonly connection tests (read existing, reject writes, refuse create). 4. **MINOR** — Extend `idx_columns_table` covering index to include `column_name` for `listColumns()` ORDER BY.
2 participants
What does this PR do?
Replaces
better-sqlite3(native addon, incompatible with Bun runtime) withbun:sqlite(built-in, zero-install) in both the schema cache and the SQLite warehouse driver. This fixesschema_index,schema_search,schema_cache_status, and SQLite connections for all users.Type of change
Issue for this PR
Closes #314
How did you verify your code works?
SchemaCache.createInMemory()→cacheStatus()→search()→close()all succeedturbo typecheck)Files changed
src/altimate/native/schema/cache.tsbetter-sqlite3→bun:sqlite, synccreate()/createInMemory()packages/drivers/src/sqlite.tsbetter-sqlite3→bun:sqlite, fix PRAGMA+LIMIT bugpackages/drivers/package.jsonbetter-sqlite3optional deppackages/opencode/script/build.tspackages/opencode/script/publish.tspackage.json@types/better-sqlite3devDepdocs/docs/drivers.mdtest/altimate/drivers-e2e.test.tsisBetterSqlite3Availableskip guardsChecklist
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores
Tests