Repository navigation
Stop a missing C++ toolchain from breaking the install - #28
Merged
Merged
Conversation
`better-sqlite3` is a native addon. On Windows with npm 10 the install died inside `node-gyp` -- and because that happens during `npx`, the user never reached the README section explaining the fix. `engines.npm >= 11` existed only to force an npm that avoided the bug. Storage now uses Node's built-in `node:sqlite`, which needs no compiler. `better-sqlite3` moves to `optionalDependencies`, so npm reports a build failure and carries on instead of failing the install, and it is only ever loaded on a Node too old for the built-in. Everything goes through `openSqlite()` in the new `src/db/driver.ts`; no other module may import a SQLite driver directly. `node:sqlite` has no `.pragma()` or `.transaction()`, so the driver supplies both, nesting transactions via savepoints the way better-sqlite3 does. The floor moves to Node >=22.13.0, which is where `node:sqlite` came out from behind `--experimental-sqlite` and where `StatementSync.iterate()` landed -- the same release, so it is one boundary rather than two. Below it the native module would be mandatory again, defeating the point. `engines.npm` is gone, and CI no longer upgrades the runner's npm, which turns an npm 10 Windows install into a real regression test. Verified: the file format is untouched, so an existing usage.db keeps working with no migration -- a 12,599-record database written by better-sqlite3 reads correctly, and `verify` reports a zero delta through either driver with byte-identical figures. A clean install with `--ignore-scripts` and with `--omit=optional` both produce a working server. One difference is deliberately not papered over: `node:sqlite` returns rows with a null prototype. Property access, spreading and JSON are identical, but inherited methods are not there. Nothing reads rows that way today; the driver documents it and a test pins it. `AI_USAGE_SQLITE_DRIVER` forces either driver, which is how the fallback stays tested on a Node that does not need it, and `ai-usage status` now reports which driver produced the numbers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
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.
Closes #27
Storage now uses Node's built-in
node:sqlite, which needs no compiler.better-sqlite3moves to
optionalDependencies, so a failed native build warns instead of failing theinstall, and it is only ever loaded on a Node too old for the built-in.
This is the fix
docs/PUBLISHING.mdalready recommended for this exact problem.Verified before writing any code
Against Node's own
doc/api/sqlite.mdhistory blocks and runtime probes on a real machine —not from memory:
node:sqliteunflagged since?--experimental-sqlite; RC since v24.15.0>=22.0.0) need the flag?{ readOnly: true }, since v22.5.0. Writes throwiterate()?iterate()landing in exactly the release that removed the flag is why the floor is oneboundary rather than two, and why the capability probe in the driver tests for
iterate()rather than sniffing versions.
Live test against a real 879 MB OpenCode database (WAL, with a 30 MB
-wal) — the"don't disturb a running OpenCode" case:
.dband-walbyte-identical afterwards; only-shm's mtime moved, which is SQLite'sshared-memory coordination and is what better-sqlite3 does too.
Design
Everything goes through
openSqlite()in the newsrc/db/driver.ts— no other moduleimports a SQLite driver directly, which is what keeps the fallback honest. Recorded as an
invariant in
CLAUDE.md.node:sqlitehas no.pragma()or.transaction(), so the driver supplies both. Transactionsnest via savepoints, matching better-sqlite3, because
migrate()runs each migration inside atransaction and an inner
BEGINwould otherwise throw.changesis coerced to a number:node:sqlitecan return a bigint there, and the repositoriesdo arithmetic on it.
Invariants
verifythrough both drivers — identical figures, zero delta, against the same realusage.db:File format untouched, no migration. The existing 12,599-record database — written by
better-sqlite3 — reads correctly under
node:sqlite:schema_migrationsintact,journal_mode=wal, real rows readable. Header is plainSQLite format 3, page size 4096,because the format is SQLite's, not the binding's.
npm run checkpasses: 151 tests (143 + 8 new).parity.test.tsuntouched and passing.The plan's "Done when": a clean install with no build step
Full MCP handshake in the
--omit=optionalinstall:initializeOK,tools/list→ 7 tools,tools/call usage_summaryOK. stdout stayed clean JSON-RPC; theExperimentalWarninggoes tostderr, as it must.
One difference deliberately not papered over
node:sqlitereturns rows with a null prototype; better-sqlite3 returns ordinary objects.Property access, spreading and
JSON.stringifyare identical, butrow instanceof Objectisfalse and
row.hasOwnPropertyis undefined. Nothing reads rows that way today (audited), sorather than wrap every row and pay for it, the driver documents the constraint and a test pins
the difference.
Notes on scope
engines.nodemoves>=22.0.0→>=22.13.0. Phase 5 said not to touch the floor, butthe stated "Done when" requires
node:sqliteto be present, and below 22.13.0 it isn't —the native module would be mandatory again. Still "22+", just patch-precise. Flagging it
because it narrows support to 22.13+ within the Node 22 LTS line.
engines.npmdropped, and CI no longer upgrades the runner's npm — so an npm 10 Windowsinstall is now a genuine regression test rather than a configuration we avoid.
AI_USAGE_SQLITE_DRIVERforces either driver. Not in the plan; added because otherwisethe fallback is untestable on a Node that doesn't need it, and it makes a suspected driver
difference diagnosable.
ai-usage statusnow printsSQLite driver:. With two possible drivers, which oneproduced the numbers is provenance, and this project already reports schema version and
pricing provenance there.