Conversation
Add a hook for the better-sqlite3 driver, alongside the existing hooks for sqlite3, mysql, pg and Prisma. Applications that use better-sqlite3 (directly or through Drizzle) recorded no SQL events before this change. The hook records Database.exec and Database.pragma, and Statement.run, get, all and iterate, as sql_query events with database_type "sqlite". Statements inside a transaction() are recorded too, including the BEGIN and COMMIT that better-sqlite3 prepares itself. A statement that throws is recorded as an exception and rethrown. Two places where this hook departs from the sqlite3 hook: - better-sqlite3 does not export its Statement class, so the Statement prototype is patched the first time prepare() returns a statement, rather than at module load. - iterate() returns rows lazily, so its return event is emitted when the iterator is exhausted, returned early, or throws. The hook hands back a plain iterator object that forwards to the native one, because the native iterator's methods cannot be called through a Proxy receiver. Everything in better-sqlite3 is synchronous, so no async context capture is needed. The test fixture pins better-sqlite3 ^11.10, the last major that still ships prebuilt binaries for Node 18, which the CI matrix runs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EgK3BVLCovf26kotqTnkau
There was a problem hiding this comment.
🟡 Changes recommended
Four unresolved moderate issues remain in the better-sqlite3 hook.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds CommonJS instrumentation for better-sqlite3, covering queries, pragmas, transactions, iteration, and exceptions.
Changes:
- Adds the
better-sqlite3require hook. - Adds fixture, integration, and snapshot coverage.
- Registers the fixture workspace and dependency lock entries.
File summaries
| File | Description |
|---|---|
yarn.lock |
Locks fixture dependencies. |
test/betterSqlite3/package.json |
Adds the fixture dependency. |
test/betterSqlite3/index.js |
Exercises supported database operations. |
test/betterSqlite3/appmap.yml |
Configures the fixture. |
test/betterSqlite3.test.ts |
Adds integration coverage. |
test/__snapshots__/betterSqlite3.test.ts.snap |
Records expected events. |
src/requireHook.ts |
Registers the new hook. |
src/hooks/betterSqlite3.ts |
Implements SQL instrumentation. |
package.json |
Registers the test workspace. |
Review details
Suppressed comments (2)
src/hooks/betterSqlite3.ts:172
- The lazy iterator has a separate exception path here, but the fixture only verifies an exception from
run()(test/betterSqlite3/index.js:35-40). Add an integration case that makesiterator.next()throw, verifies the error is rethrown, and snapshots the resulting SQL exception event so this forwarding logic cannot regress unnoticed.
} catch (exn: unknown) {
finish(exn ?? new Error("iteration failed"));
throw exn;
src/hooks/betterSqlite3.ts:56
- The private transaction controller does not use
Database.prototype.prepare; it calls the native database'sprepareto create the transaction statements before the returned transaction function runs. On a fresh database,before.run()therefore executes before any userdb.prepare()can patch this shared Statement prototype, soBEGINis not recorded (and other controller statements can likewise be missed when the callback does not prepare a statement). The fixture masks this by preparinginsertfirst; initialize the Statement prototype before transaction setup and add a fresh-database regression test.
function patchStatementPrototype(statement: object) {
const proto: unknown = Object.getPrototypeOf(statement);
if (proto === null || typeof proto !== "object" || patchedStatementPrototypes.has(proto)) return;
patchedStatementPrototypes.add(proto);
- Files reviewed: 8/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+15
to
+17
| export default function betterSqlite3Hook(mod: unknown) { | ||
| if (typeof mod !== "function" || typeof mod.prototype !== "object" || mod.prototype === null) | ||
| return mod; |
Comment on lines
+175
to
+181
| return(...args: unknown[]) { | ||
| try { | ||
| return native.return ? native.return(...args) : { done: true, value: undefined }; | ||
| } finally { | ||
| finish(); | ||
| } | ||
| }, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applications that use better-sqlite3, directly or through Drizzle, record no SQL events. This adds a hook for it next to the sqlite3, mysql, pg and Prisma hooks.
What is recorded
Database.execandDatabase.pragmaStatement.run,get,allanditerate, with the statement'ssourceas the SQLtransaction(), including theBEGINandCOMMITthat better-sqlite3 prepares itselfWhere this departs from the sqlite3 hook
Statement. The Statement prototype is patched the first timeprepare()returns one, not at module load.iterate()returns rows lazily. Its return event is emitted when the iterator is exhausted, returned early, or throws. The hook returns a plain iterator that forwards to the native one, because the native iterator's methods cannot be called through a Proxy receiver.Test
test/betterSqlite3: a fixture that runs each method, one statement twice, a transaction, an earlybreakout ofiterate(), and a caught unique violation. Snapshot test intest/betterSqlite3.test.ts.^11.10, the last major with prebuilt binaries for Node 18, which CI runs.Not covered
require. Animport Database from "better-sqlite3"in an ESM application is not hooked. The other driver hooks have the same limit.pragma()should be recorded at all is a judgement call. It is included becausePRAGMAstatements change behavior and are cheap to record.This was run against one real application, promptfoo, where it recorded the Drizzle inserts and selects that were previously missing.
Written by Claude in a Claude Code session for Elizabeth Lawler. The commit carries Claude as author.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EgK3BVLCovf26kotqTnkau
Generated by Claude Code