nodejs-github-bot · GitHub

Fleabag515 added a commit to Fleabag515/Pleiades that referenced this pull request

@Fleabag515

Blocker found while scoping Phase 2 of the vendoring plan (cross-platform
native-dep packaging, docs/specs/2026-07-25-anamnesis-vendoring-and-
installer-plan.md): better-sqlite3 has no prebuilt binary for any
currently-supported Node version. Verified directly against
WiseLibs/better-sqlite3's GitHub releases -- v9.6.0 (the pinned version)
tops out at node-v120 (Node 21); v11.x/v12.x never got past node-v115
(Node 20); v13.0.1 (latest) dropped prebuilt distribution entirely, install
script is now bare `node-gyp rebuild`. Every currently-supported Node LTS
(22, 24) has zero prebuild coverage -- confirmed why `npm install` on this
machine fell back to a genuine local node-gyp compile (no prebuilds/ dir,
build/Release/obj.target/ intermediate files present, live daemon process
had build/Release/better_sqlite3.node mapped, not a prebuilds/ path).
That's the one dependency standing between "download the installer, done"
and "needs a C++ toolchain" for anyone who isn't on this exact dev machine
-- unacceptable for the vendoring effort's own goal. Considered and
rejected: self-building+vendoring better-sqlite3's binary per platform
(keeps the per-ABI treadmill forever, adds ongoing release-cutting burden);
libsql (better-sqlite3-compatible API, upstream-published ABI-stable
binary, but it's a SQLite fork -- worse bet for a data-critical store than
paying a small migration cost to stay on stock SQLite).
node:sqlite ships inside Node itself (RC stability as of v25.7.0/v24.15.0,
unflagged since v22.13.0/v23.4.0 -- nodejs/node#61262), so pinning the
bundled Node runtime (Phase 2b) permanently ends the native-addon-per-
platform problem for this dependency. Migration, verified end to end:
- New src/lib/sqlite-tx.js: runInTransaction(db, fn) -- node:sqlite's
  DatabaseSync has no .transaction() sugar. BEGIN/fn()/COMMIT, rollback +
  rethrow on any throw. Replaces the 3 real .transaction() call sites
  (history.js:352 updateDecayScores, history.js:564 mergeSessions --
  atomically repoints session_key across 5 tables, the corruption-critical
  one -- and importers/index.js's bulk import).
- .pragma('x = y') -> .exec('PRAGMA x = y') at all 4 call sites
  (history.js x2, cli.js, importers/index.js).
- Two DatabaseSync constructor defaults explicitly overridden everywhere a
  connection opens, because getting either wrong changes real behavior
  silently: enableForeignKeyConstraints:false (node defaults this true;
  better-sqlite3 left it off matching SQLite's own default, and nothing
  here has ever run `PRAGMA foreign_keys` -- the schema's
  `ON DELETE CASCADE` refs have been inert in production the whole time.
  Migrating onto node's default would make prune() start cascade-deleting
  a pruned turn's engrams/foresights, a real change to what memory
  survives a prune, not something to flip on as a migration side effect)
  and timeout:5000 + a matching `PRAGMA busy_timeout=5000` belt-and-
  suspenders (node defaults busy-timeout to 0 -- immediate SQLITE_BUSY on
  any lock contention -- vs better-sqlite3's 5000ms default; cli.js opens
  its own separate handle for repair commands like reembed/status while
  the daemon may be writing, so cross-process WAL contention is real;
  independently confirmed via nodejs/node#57597, a user hitting exactly
  this "database is locked" default).
- readonly:true -> readOnly:true (cli.js's status-check handle) -- verified
  the correct camelCase option name rather than assuming better-sqlite3's
  own casing carried over.
- Verified everything empirically against real Node 22.22.2 before editing
  any source: .run() returns {lastInsertRowid, changes} matching better-
  sqlite3's shape; BLOB columns round-trip as Uint8Array (history.js's
  toFloat32() already decodes via .buffer/.byteOffset/.byteLength, which
  Buffer and Uint8Array both expose -- no change needed there); a
  runInTransaction() probe commits on success and rolls back cleanly with
  zero partial rows on a thrown error; readOnly:true genuinely rejects
  writes.
- 2 new targeted tests in test/history.test.js: mergeSessions rolls back
  atomically on a mid-merge throw (drops character_observations mid-loop
  to force one), and commits all 5 tables together on success. Replaced a
  3rd planned test (cross-handle busy-timeout wait via a same-thread
  setTimeout racing a blocking DatabaseSync call) after recognizing it
  would deadlock the single JS thread instead of testing anything -- kept
  a simpler, correct assertion that busy_timeout is actually configured to
  5000 instead.
- Removed the test suite's better-sqlite3-native-binding-unavailable skip
  guard (test/history.test.js) -- node:sqlite can't fail to load the way a
  native addon could, the guard no longer has a reason to exist.
- package.json: removed the now-unused better-sqlite3 dependency (30
  packages dropped from node_modules on reinstall; the real size -- 1.6GB
  -- is unchanged, that's node-llama-cpp's 6 platform variants +
  onnxruntime-node, untouched by this change and still Phase 2's actual
  remaining size problem).
Full existing suite (213 tests) + the 3 new ones: 216/216 pass. Also
booted the real daemon.js end-to-end against an isolated scratch HOME
(separate control port, no contact with production data) and confirmed a
clean health-check response with zero errors beyond the expected
ExperimentalWarning.
Advisory research for this decision done via a single Agent(model:"opus")
consult per the current council-agent-choice policy; every specific claim
it made (file:line citations, node:sqlite default values, ABI/version
history) was independently re-verified against the real source and Node's
own docs/issue tracker before acting on it, not taken on faith.

Read the original on github.com ↗