Use better-sqlite3 instead of @db/sqlite

[corrideat]

Closes #150

Before merging this PR, we need to ensure that tests pass on GI (using the SQLite backend)

A known issue with this PR as it stands right now is that the bundle will contain all prebuilt SQLite binaries (e.g., the Linux bundle will include Windows code and vice versa). This is not directly harmful but makes the bundle larger than it needs to be.

AI disclosure: Used Opus 5 for code generation and planning

[corrideat]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.3

Click to expand review

Now let me verify the environment and inspect the installed better-sqlite3 package:Now let me verify Deno's --include/--exclude precedence semantics for deno compile, since the change relies on includes overriding the node_modules exclude:The docs are inconclusive on precedence. Let me check Deno's actual implementation:The include/exclude interplay in the binary writer is the crux. Let me check how exclude_paths is applied when embedding data files:The include/exclude mechanics check out (exclude is an exact-path match; the npm-snapshot embedding of the whole node_modules is blocked by --exclude node_modules, while the explicit deeper --include paths are added afterwards and embed only the chosen subtrees). Now let me check the remaining pieces:## Review

1. 🔴 Binary-cache fingerprint ignores the embedded native-addon files — stale binaries will be reused after a better-sqlite3 version bump

• Addressed
• Dismissed

scripts/binaries.ts:146-155 computes the cache fingerprint from build/ contents, the Deno version, and COMPILE_FLAGS only:

export async function computeFingerprint (dir: string = BUILD_DIR): Promise<string> {
  // Using NUL because `fileDigests` could contain
  // most other characters
  const payload = [
    `deno:${Deno.version.deno}`,
    `flags:${COMPILE_FLAGS}`,
    ...(await fileDigests(dir)).sort()
  ].join('\0')
  return await sha256(new TextEncoder().encode(payload))
}

But after this PR the binaries also embed files from node_modules/better-sqlite3/{package.json,lib,prebuilds} (scripts/targets.ts:132-136, embedded via --include in COMPILE_FLAGS and mtime-pinned in binaries.ts:247-249). None of those files are hashed:

• The bundle keeps the version-less specifier import Database from "npm:better-sqlite3" (build/main.js:1000), so bumping the version in deno.json/deno.lock does not change build/.
• NATIVE_ADDON_PATHS deliberately goes through the version-less symlink, so COMPILE_FLAGS doesn't change either.

Result: bump better-sqlite3 (e.g. a SQLite security update), run deno task dist + npm publish, and ensureBinaries() will report Reusing dist/bin/... (unchanged inputs) while shipping the old addon bytes. The module header at binaries.ts:11-14 ("a fingerprint of every file the binary embeds (the whole build/ bundle)") is now inaccurate, and the targets.ts:128-131 comment only covers the mtime half of the problem. Suggested fix — fold digests of the addon paths into the payload:

// `<relative path>\t<sha256 of contents>` for a path that may be a file or a
// directory; missing paths contribute nothing.
async function pathDigests (path: string): Promise<string[]> {
  let info: Deno.FileInfo
  try {
    info = await Deno.lstat(path)
  } catch (e) {
    if (isNotFound(e)) return []
    throw e
  }
  if (info.isDirectory) return fileDigests(path)
  if (info.isFile) return [`${path}\t${await sha256(await Deno.readFile(path))}`]
  return []
}

export async function computeFingerprint (dir: string = BUILD_DIR): Promise<string> {
  const payload = [
    `deno:${Deno.version.deno}`,
    `flags:${COMPILE_FLAGS}`,
    ...(await fileDigests(dir)).sort(),
    ...(await Promise.all(NATIVE_ADDON_PATHS.map(pathDigests))).flat().sort()
  ].join('\0')
  return await sha256(new TextEncoder().encode(payload))
}

and update the header comment to mention the native-addon inputs.

2. 🟡 DRY: the native-addon package list is maintained in two places

• Addressed
• Dismissed

scripts/build.ts:18:

const nativeAddonPackages = ['npm:better-sqlite3']

and scripts/targets.ts:132-136:

export const NATIVE_ADDON_PATHS: readonly string[] = [
  'node_modules/better-sqlite3/package.json',
  'node_modules/better-sqlite3/lib',
  'node_modules/better-sqlite3/prebuilds'
] as const

Adding a second native-addon package requires remembering to update both lists (one with the npm: prefix, one with directory paths); missing the second silently drops the .node binary from compiled releases. Since scripts/paths.ts is already the dependency-free module both scripts import, derive both from one list there:

// paths.ts
export const NATIVE_ADDON_PACKAGES: readonly string[] = ['better-sqlite3']

// build.ts
import { NATIVE_ADDON_PACKAGES } from './paths.ts'
const nativeAddonPackages = NATIVE_ADDON_PACKAGES.map((name) => `npm:${name}`)

// targets.ts
export const NATIVE_ADDON_PATHS: readonly string[] = NATIVE_ADDON_PACKAGES.flatMap((name) => [
  `node_modules/${name}/package.json`,
  `node_modules/${name}/lib`,
  `node_modules/${name}/prebuilds`
])

3. 🟡 Stale @db/sqlite reference left in AGENTS.md

• Addressed
• Dismissed

AGENTS.md:310:

- `@db/sqlite` - SQLite bindings for Deno

should now read, e.g. - better-sqlite3 - SQLite bindings (native addon, prebuilt binaries).

4. ⚪️ database-sqlite.ts is never type-checked by CI, and its ad-hoc statement types can silently drift

• Addressed
• Dismissed

The backend is only reachable through dynamic imports (src/serve/database.ts:226, src/serve/database-router.ts:42), deno task check only covers scripts/ (deno.json:5), and the route tests run with the mem backend (routes-test-helpers.ts:135-138). So the hand-rolled types at src/serve/database-sqlite.ts:14-18 are unverified — the iter() → iterate() rename fixed in this very PR is of the class of error nothing in CI would catch (iterKeys() isn't exercised by the router test either). Consider typing the fields with the real types now that they're installed:

import type { Database as SQLiteDB, Statement } from 'npm:@types/better-sqlite3'

and/or extend the check task, e.g. "check": "deno check scripts/ src/serve/database-sqlite.ts".

5. ⚪️ Stale comment in readData contradicts the new CAST

• Addressed
• Dismissed

src/serve/database-sqlite.ts:64-66:

    // 'row' will be undefined if the key was not found.
    // Note: sqlite remembers the type of every stored value, therefore we
    // can return the value as-is.

With the CAST(value AS BLOB) added at database-sqlite.ts:48, values always come back as raw bytes, so "sqlite remembers the type ... return the value as-is" no longer describes the behavior. Suggest replacing the second sentence with "the CAST above guarantees a Buffer regardless of how the value was stored."

6. ⚪️ Misleading wording in the --allow-sys comment

• Addressed
• Dismissed

scripts/targets.ts:152-156:

// that report behind those two extra `sys` scopes. Both are already implied by
// the unrestricted network access the server needs.

cpus/networkInterfaces are not "implied by" network access; the intended point is that they're no more sensitive. Suggest: "Both expose nothing that unrestricted network access doesn't already allow probing."

7. ⚪️ Dev entry point still lacks --allow-ffi, which the sqlite backend now needs

• Addressed
• Dismissed

src/main.ts:1:

#!/usr/bin/env -S deno run --allow-net --allow-read=. --allow-write=. --allow-sys --allow-env

Loading a native .node addon requires --allow-ffi (the deno task chel task already includes it, deno.json:3). Running ./src/main.ts — the invocation documented in AGENTS.md/README — fails whenever the sqlite backend is selected. Pre-existing (the old @db/sqlite FFI had the same requirement), but since this PR touches the permission story anyway, adding --allow-ffi to the shebang would make the documented dev workflow work with the new backend.

---

Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

[corrideat]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.3

Click to expand review

Review: better-sqlite3 switch + per-target binary bundling

I read the full diff (including the parts beyond the truncation: src/serve/database-sqlite.ts, src/migrate.ts, all new test files, scripts/binaries.ts/targets.ts/build.ts) and cross-checked claims against the committed build/main.js, deno.lock, and the npm registry (the @types/better-sqlite3@9.6.0 "newest typing" comment in deno.json is accurate — verified latest is 9.6.0). The core swap, the fingerprint/mtime symlink handling, and the same-file migrate guard all look correct. Issues found, most to least important:

---

1. 🟡 tracked-tests.test.ts derives the repo root via URL.pathname — breaks deno task test on Windows and on paths with spaces/non-ASCII characters

• Addressed
• Dismissed

scripts/tracked-tests.test.ts:44

const root = new URL('..', import.meta.url).pathname

.pathname of a file: URL is not a filesystem path: on Windows it yields /C:/Users/... (which Deno.readDir cannot resolve), and on every platform it percent-encodes special characters (/home/user/my chel/ becomes /home/user/my%20chel/). Unlike normalizeMtimes, the walker testFilesUnder has no NotFound guard (scripts/tracked-tests.test.ts:15-27), so the readDir throw propagates and the whole test suite fails — on a project that explicitly ships and supports win32-x64 binaries. Note the sibling tests already do this correctly by passing the URL object itself (e.g. scripts/bundle.test.ts:6, scripts/targets.test.ts); only this file unwraps it.

import { assertEquals } from 'jsr:@std/assert'
import { fromFileUrl } from 'jsr:@std/path/'
// ...
Deno.test('every test file is committed', async () => {
  const root = fromFileUrl(new URL('..', import.meta.url))
  // ...
})

---

2. 🟡 --allow-sys=hostname,cpus,networkInterfaces is baked into every target, though the glibc-vs-musl probe it enables only runs on Linux

• Addressed
• Dismissed

scripts/targets.ts:166-170

const BASE_COMPILE_FLAGS =
  '--allow-env --allow-ffi --allow-sys=hostname,cpus,networkInterfaces --allow-read ' +
  '--allow-write=./ --allow-net ' +

The comment at scripts/targets.ts:147-151 states the probe runs "on Linux", and that matches better-sqlite3's binding lookup (musl detection via process.report is guarded by process.platform === 'linux'; darwin/win32 pick prebuilds/<platform>-<arch>.node directly). Yet the widened sys scopes — which expose networkInterfaces (MAC/interface addresses) to every command, database or not — are baked into the macOS and Windows binaries where they can never be needed. The permission structure is already per-target (compileFlags(target)), so the scopes can be too:

// The glibc/musl probe better-sqlite3 performs only exists on Linux; see the
// TODO above about dropping these scopes entirely.
function sysFlags (target: Target): string {
  return target.os === 'linux'
    ? '--allow-sys=hostname,cpus,networkInterfaces'
    : '--allow-sys=hostname'
}

export function compileFlags (target: Target): string {
  const includes = nativeAddonPaths(target).map((p) => `--include ./${p}`).join(' ')
  return `${BASE_COMPILE_FLAGS} ${sysFlags(target)} ${includes}`
}

with BASE_COMPILE_FLAGS no longer containing an --allow-sys token, and the pinning test at scripts/targets.test.ts:313-318 updated to assert the per-OS expectation. Worth confirming each platform with CHEL_SMOKE_COMPILE=1 deno task smoke on the respective hosts before merging, since the smoke test only covers the host target.

---

3. 🟡 DRY: test scaffolding copied across four new test files, with drift already visible

• Addressed
• Dismissed

The same helpers are re-declared per file:

• withTempDir at scripts/binaries.test.ts:26, scripts/binaries-symlinks.test.ts:26-36, and src/migrate-symlinks.test.ts:33-40 — and the copies already disagree: the migrate one resolves the temp dir to an absolute path (with a comment explaining why links would otherwise dangle), the other two don't.
• const FLAGS = '--fake-compile-flags' at scripts/binaries.test.ts:18 and scripts/binaries-symlinks.test.ts:12.
• const canSymlink = Deno.permissions.querySync(...) at scripts/binaries-symlinks.test.ts:21 and src/migrate-symlinks.test.ts:21.
• exists() at src/migrate-symlinks.test.ts:48-55 and inline in scripts/targets.test.ts.

A single committed helper (not named *.test.ts, so deno test won't execute it as a suite) removes the drift:

// scripts/test-utils.ts
import { resolve } from 'jsr:@std/path/'

export const FAKE_COMPILE_FLAGS = '--fake-compile-flags'

export const canSymlink = Deno.permissions.querySync({ name: 'write' }).state === 'granted'

// Resolved to an absolute path: a relative symlink target resolves against the
// link's own directory, not the CWD, so relative temp dirs produce dangling links.
export async function withTempDir (fn: (dir: string) => Promise<void>): Promise<void> {
  await Deno.mkdir('./test/temp', { recursive: true })
  const dir = resolve(await Deno.makeTempDir({ dir: './test/temp' }))
  try {
    await fn(dir)
  } finally {
    await Deno.remove(dir, { recursive: true })
  }
}

---

4. 🟡 DRY: SQLITE_DEFAULT_FILEPATH mirrors two separate defaults living in another module

• Addressed
• Dismissed

src/migrate.ts:21-23

// Mirrors SqliteBackend's own defaults (dataFolder 'data', filename
// 'chelonia.db'), so an omitted filepath compares the same way the backend
// would resolve it.
const SQLITE_DEFAULT_FILEPATH = 'data/chelonia.db'

vs src/serve/database-sqlite.ts:60-62:

  dataFolder: string = 'data'
  db: null ...
  filename: string = 'chelonia.db'

If anyone edits the backend defaults, this constant silently diverges and the same-file guard stops firing (the drift produces exactly the deadlock the guard exists to prevent). backend-schemas.ts is already the designated dependency-free shared module ("so that validateConfig.ts can compose them without pulling in the backends' heavy native/runtime imports"), so it is the right home:

// src/serve/backend-schemas.ts
export const SQLITE_DEFAULT_FILEPATH = 'data/chelonia.db'

// src/serve/database-sqlite.ts — constructor
const resolvedPath = resolve(filepath ?? SQLITE_DEFAULT_FILEPATH)
this.dataFolder = dirname(resolvedPath)
this.filename = basename(resolvedPath)

// src/migrate.ts
import { SQLITE_DEFAULT_FILEPATH } from './serve/backend-schemas.ts'

(The dataFolder/filename field initializers can then be dropped or kept as documentation of the resolved shape.)

---

5. ⚪️ Fingerprint walk tolerates a missing top-level path but not a file/dir vanishing mid-walk — inconsistent with normalizeMtimes

• Addressed
• Dismissed

scripts/binaries.ts:164-168 and scripts/binaries.ts:185:

  for await (const entry of Deno.readDir(dir)) {   // no NotFound guard

  const real = await Deno.realPath(path)           // not wrapped in try/catch

normalizeMtimes deliberately wraps its readDir in isNotFound handling (scripts/binaries.ts:95-100), and digestsAt treats a missing input path as "contributes nothing". But a concurrent npm install touching node_modules between stat and realPath/readFile/readDir inside the walk throws an uncaught NotFound and aborts the release. Either wrap these the same way, or (also defensible) let it fail loudly — but then normalizeMtimes' guard and the "missing path contributes nothing" rule are doing something different from the hasher for the same race:

  let real: string
  try {
    real = await Deno.realPath(path)
  } catch (e) {
    if (isNotFound(e)) return []
    throw e
  }

(and the analogous guard around Deno.readDir in fileDigests / Deno.readFile in the non-directory branch of digestsAt).

---

6. ⚪️ The chel migrate same-file guard is bypassed when either side goes through the router backend

• Addressed
• Dismissed

src/migrate.ts:52-58:

export function isSameSqliteFile (
  fromBackend: unknown, toBackend: unknown, fromOptions: unknown, toOptions: unknown
): boolean {
  if (fromBackend !== 'sqlite' || toBackend !== 'sqlite') return false

A router source whose entries include a sqlite backend on the same file (or a router target with a sqlite * entry pointing at the source file) walks straight into the deadlock the guard documents — database-router.ts:90-94 delegates iterKeys to the embedded sqlite backend, which holds the same read cursor. Resolving this fully means inspecting database:backendOptions:router entries for sqlite; if that's out of scope here, at least a sentence in the guard's comment (or the --to-config error message) saying the guard covers direct sqlite→sqlite only would set expectations correctly.

---

7. ⚪️ README overstates glibc linkage: the chel binaries aren't glibc-linked, only the embedded SQLite addon is

• Addressed
• Dismissed

README.md:487-494:

The binaries are linked against glibc, so Linux distributions built on musl
libc (Alpine, for example) are **not** covered: the SQLite backend's native
addon each binary carries is the glibc build for its platform.

A deno compile executable is self-contained; the glibc-linked component is the addon, as the second clause itself explains. The first sentence will confuse anyone diagnosing ldd chel output on musl. Suggested rewording:

The SQLite native addon embedded in each binary is linked against glibc, so
Linux distributions built on musl libc (Alpine, for example) are **not**
covered. Running `chel` from source with Deno works there instead. Only the
SQLite backend is affected, ...

(The same distinction is made correctly in scripts/paths.ts and src/serve/database-sqlite.ts:37-39.)

---

8. ⚪️ AGENTS.md's scripts/*.test.ts inventory is already stale

• Addressed
• Dismissed

AGENTS.md:100-101:

└── *.test.ts            # Build-script tests (binaries, compile, launcher,
                         #   sync-versions, targets)

The directory actually also contains binaries-symlinks.test.ts, bundle.test.ts, and tracked-tests.test.ts (the last of which the same file's prose references by name two sections earlier). Update the parenthetical:

├── *.test.ts            # Build-script tests (binaries, binaries-symlinks,
│                         #   bundle, compile, launcher, sync-versions,
│                         #   targets, tracked-tests)

---

9. ⚪️ Naming/typing nits: addonLoadError and the unknown parameters of isSameSqliteFile

• Addressed

• Dismissed

• src/serve/database-sqlite.ts:47 — addonLoadError(cause) doesn't return the load error; it constructs a reworded one (or null). A reader expecting an accessor/guard will be surprised. rewordedAddonLoadError (or addonLoadFailureHint) says what it does.

• src/migrate.ts:49-56 — typing the parameters as unknown forces the cast at the call site of filepathOf:

  export function isSameSqliteFile (
    fromBackend: string | undefined,
    toBackend: string | undefined,
    fromOptions: { filepath?: string } | undefined,
    toOptions: { filepath?: string } | undefined
  ): boolean {
    if (fromBackend !== 'sqlite' || toBackend !== 'sqlite') return false
    const filepathOf = (options?: { filepath?: string }) =>
      canonicalFilepath(options?.filepath || SQLITE_DEFAULT_FILEPATH)
    return filepathOf(fromOptions) === filepathOf(toOptions)
  }

  All existing call sites (src/migrate.ts:100-105, both test files) already pass values matching these types, so nothing else changes.

---

10. ⚪️ The canSymlink skip relies on Deno ≥ 2.1 granted_partial semantics

• Addressed
• Dismissed

scripts/binaries-symlinks.test.ts:21 / src/migrate-symlinks.test.ts:21:

const canSymlink = Deno.permissions.querySync({ name: 'write' }).state === 'granted'

Under the main deno task test run (write scoped to ., $HOME/.cache, …) this correctly yields granted_partial on Deno ≥ 2.1 (CI pins v2.5.3). On Deno 2.0.x, however, the un-scoped descriptor query reported granted for path-scoped grants, so the suites would attempt real symlink work inside the main pass and fail with NotCapable. If supporting 2.0.x contributors matters, probe by behavior instead of by permission state (attempt Deno.symlink on a scratch file and catch), or document the minimum Deno version next to the task in deno.json.

---

Not issues, verified for the record: the @types/better-sqlite3@9.6.0 pin rationale in deno.json (registry confirms 9.6.0 is latest, published 2026-08-01); the committed build/main.js:1000 keeps npm:better-sqlite3 external; bin/chel.js message strings match the new scripts/launcher.test.ts assertions; no lingering @db/sqlite/@denosaurs/plug references anywhere; the migrate.ts guard's --from wiring via .alias('database:backend', 'from') is correct; and the symlink-cycle guards in normalizePathMtimes/digestsAt terminate correctly on the self-referential-link cases the new tests construct.

---

Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

[corrideat]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.3

Click to expand review

I'll start by exploring the repository state and the changed files to review the complete diff.The PR range is 5be6a25..HEAD. Let me read the key changed files, starting with the core SQLite backend.Now I need to verify a critical semantic of better-sqlite3's iterate() — whether reads (not just writes) are blocked while a cursor is open, since chel migrate reads each key from the source backend mid-iteration:The better-sqlite3 semantics check out: .get()/.all() on other statements is allowed during iteration (only REQUIRE_STATEMENT_NOT_LOCKED), while .run() additionally checks REQUIRE_DATABASE_NO_ITERATORS — exactly matching the PR's documented constraint. Let me verify the v13 package layout claims:Let me get exact line numbers in README.md and AGENTS.md for the passages I'll reference:I've completed a thorough review of the full PR range (5be6a25..HEAD: the @db/sqlite → better-sqlite3 switch, per-target addon embedding, the binary-cache rework, the migrate same-file guard, and all the new tests). I verified the load-bearing external claims against better-sqlite3's actual source (v13's prebuilds/${platform}-${arch}.node convention, lazy addon resolution, and the exact busy executing a query semantics — .get()/.all() on other statements are permitted during iteration while .run() throws, which matches this PR's documented constraint and is what makes chel migrate --from sqlite viable).

Review

1. 🟡 Fingerprint sorting invariant is split across two layers — latent nondeterministic cache keys

• Addressed
• Dismissed

scripts/binaries.ts:231-244 sorts the addon digests inside fingerprintOf, but silently relies on the bundle digests arriving pre-sorted from the call sites:

async function fingerprintOf (
  bundleDigests: readonly string[],
  addonDigests: readonly string[],
  flags: string
): Promise<string> {
  const payload = [
    `deno:${Deno.version.deno}`,
    `flags:${flags}`,
    ...bundleDigests,
    // Prefixed so an addon entry can never collide with a `build/` one.
    ...addonDigests.map((digest) => `addon:${digest}`).sort()
  ].join('\0')

The sorting currently lives at scripts/binaries.ts:265 ((await fileDigests(dir)).sort()) and scripts/binaries.ts:295 (fileDigests(BUILD_DIR).then((digests) => digests.sort())). fileDigests returns lines in readDir order, which is filesystem-dependent (stable on ext4, not guaranteed on network/FUSE filesystems), so this is a correctness invariant of the cache key — and half of it is enforced in a different function than the other half. A future call site that forgets the .sort() would get nondeterministic fingerprints and pointless recompiles (or worse, "fresh" cache hits on a machine where the order happens to match the stamp).

Suggestion — enforce it in one place and simplify both call sites:

async function fingerprintOf (
  bundleDigests: readonly string[],
  addonDigests: readonly string[],
  flags: string
): Promise<string> {
  const payload = [
    `deno:${Deno.version.deno}`,
    `flags:${flags}`,
    ...[...bundleDigests].sort(),
    ...addonDigests.map((digest) => `addon:${digest}`).sort()
  ].join('\0')
  return await sha256(new TextEncoder().encode(payload))
}

Then scripts/binaries.ts:265 becomes return await fingerprintOf(await fileDigests(dir), addons, flags) and line 295 becomes cachedBundleDigests ??= fileDigests(BUILD_DIR).

2. 🟡 README overstates glibc scope: only the Linux binaries are glibc-linked

• Addressed
• Dismissed

README.md:487-489:

The binaries are linked against glibc, so Linux distributions built on musl
libc (Alpine, for example) are **not** covered: the SQLite backend's native
addon each binary carries is the glibc build for its platform.

Three of the five published targets (Windows x64, macOS x64/arm64) are not linked against glibc at all, so "The binaries are linked against glibc" is false as written, and the nested genitive ("the SQLite backend's native addon each binary carries") is hard to parse. Suggested rewrite:

The Linux binaries are linked against glibc, so Linux distributions built on
musl libc (Alpine, for example) are **not** covered: the native SQLite addon
each Linux binary carries is the glibc build for its platform.

3. 🟡 The migrate same-file error only suggests --to-config, but the collision can be introduced (and fixed) on the source side

• Addressed
• Dismissed

src/migrate.ts:153-159:

      exit(
        `Source and target both use the SQLite database file ${sharedFile}. ` +
        'Migrating a database onto itself cannot work: the source is read ' +
        'through a cursor that keeps its connection busy for the whole run, so ' +
        'the writes are refused rather than landing. Pass a different filepath ' +
        'with --to-config.'
      )

Two problems:

• The guard inspects both sides (e.g. --from router with a nested sqlite entry colliding with the target — a case your own migrate.test.ts covers), so repointing the source via --from-config is an equally valid fix, yet the message only mentions --to-config. Also, when no --to-config is passed the target inherits chel.toml's [database.backendOptions], so "pass a different filepath" undersells the options.
• "so the writes are refused rather than landing" mixes a passive with an active participle.

Suggested message:

      exit(
        `Source and target both use the SQLite database file ${sharedFile}. ` +
        'Migrating a database onto itself cannot work: the source is read ' +
        'through a cursor that keeps its connection busy for the whole run, so ' +
        'writes are rejected rather than applied. Point the source or the ' +
        'target at a different file (for example with --from-config or ' +
        '--to-config).'
      )

4. ⚪️ database-sqlite.ts: duplicated "not open" message and vague stmt helper name

• Addressed
• Dismissed

src/serve/database-sqlite.ts:89-97 repeats the same message string in two guards:

  private openDb (): SQLiteDB {
    if (!this.db) throw new Error(`The ${this.filename} SQLite database is not open.`)
    return this.db
  }

  private stmt<T> (statement: T | null): T {
    if (!statement) throw new Error(`The ${this.filename} SQLite database is not open.`)
    return statement
  }

stmt reads like it creates a statement, but it is a non-null assertion helper. Suggestion:

  private notOpenError (): Error {
    return new Error(`The ${this.filename} SQLite database is not open.`)
  }

  private openDb (): SQLiteDB {
    if (!this.db) throw this.notOpenError()
    return this.db
  }

  private requireStatement<T> (statement: T | null): T {
    if (!statement) throw this.notOpenError()
    return statement
  }

5. ⚪️ New tests re-implement withTempDir scaffolding that test/test-helpers.ts now exports

• Addressed
• Dismissed

test/test-helpers.ts was introduced in this PR specifically to deduplicate temp-dir handling ("the scaffolding that was previously copy-pasted per file", test/test-helpers.ts:7-9), but two new files still roll their own:

test/compile-smoke.test.ts:39-40:

    await Deno.mkdir('./test/temp', { recursive: true })
    const cwd = await Deno.makeTempDir({ dir: './test/temp' })

src/serve/database-sqlite.test.ts:18-19:

  const dir = path.join(path.resolve('test/temp'), `sqlite-backend-${Date.now()}-${tempCounter++}`)
  await Deno.mkdir(dir, { recursive: true })

The smoke test's try/finally + Deno.remove(cwd, { recursive: true }) is exactly withTempDir (the smoke task's --allow-write=. covers it):

import { withTempDir } from './test-helpers.ts'
// ...
await withTempDir(async (cwd) => {
  const { code, stdout, stderr } = await new Deno.Command(
    await Deno.realPath(binaryPath(hostTarget)),
    { args: ['migrate', '--from', 'mem', '--to', 'sqlite'], cwd, /* ... */ }
  ).output()
  // assertions...
})

For withBackend, wrapping withTempDir and deriving const filepath = ${dir}/chelonia.db`` would likewise drop the manual counter, the path.resolve('test/temp') special case, and the cleanup `catch`.

6. ⚪️ existsSync as exists import alias shadows the name of a different exported helper

• Addressed
• Dismissed

scripts/targets.test.ts:15:

import { existsSync as exists } from '../test/test-helpers.ts'

test/test-helpers.ts exports both an async exists (line 78) and existsSync (line 87) with deliberately different error semantics; aliasing the sync one to the async one's name invites exactly the confusion the two distinct names exist to prevent. Prefer using existsSync directly at its three call sites (scripts/targets.test.ts:273,277,287). Relatedly, scripts/binaries.ts:73-81 now has a third, private exists that rethrows non-NotFound errors — if the differing semantics are intentional (they seem to be), that contrast is worth one line of comment in test/test-helpers.ts so nobody "unifies" them later.

7. ⚪️ Minor: ensureBinaries awaits targetFingerprint(target) twice per stale target

• Addressed
• Dismissed

scripts/binaries.ts:395 computes it for the freshness check and scripts/binaries.ts:422 awaits it again inside the rebuild loop. It's free thanks to memoization, but collecting the values the first time through would make the dependency explicit:

  const fingerprints = new Map<string, string>()
  const stale: Target[] = []
  for (const target of targets) {
    const fingerprint = await targetFingerprint(target)
    fingerprints.set(target.denoTarget, fingerprint)
    if (await isFresh(stampPath('bin', target), fingerprint, binaryPath(target))) {
      console.log(`Reusing ${binaryPath(target)} (unchanged inputs)`)
    } else {
      stale.push(target)
    }
  }

and then await rebuildWithStamp(stampPath('bin', target), fingerprints.get(target.denoTarget)!, ...).

8. ⚪️ The musl-diagnostic error message would misfire on non-Linux broken installs

• Addressed
• Dismissed

src/serve/database-sqlite.ts:54-58 attributes any addon-load failure naming better_sqlite3.node/prebuilds/ to glibc-vs-musl ("The pre-built binaries chel ships are linked against glibc..."), same over-scope as issue 2. On Windows, a deleted/corrupt embedded prebuild would produce this message and send the user chasing libc. Since rewordedAddonLoadError already receives only the message, gating the musl text on Deno.build.os === 'linux' (falling back to a generic "the embedded SQLite addon could not be loaded; reinstall @chelonia/cli") would make the diagnosis right wherever it fires. Low priority because the non-Linux trigger requires a manually damaged install.

---

Everything else I verified held up under scrutiny, so I won't pad the list: the per-target fingerprint/isolation invariants and their tests are mutually consistent; the --exclude node_modules / --include ordering claim matches Deno's documented behavior and is pinned by scripts/targets.test.ts:249-262; the prebuild filename mapping in scripts/paths.ts exactly mirrors better-sqlite3 v13's lib/binding.js; the same-file guard's "nonexistent path can't be the open source file" reasoning is sound (on case-insensitive filesystems any spelling of an existing file resolves via realpath); the omit-tests esbuild plugin leaves the real backends loadable (confirmed against the committed build/main.js); and the --allow-sys=cpus,networkInterfaces widening is deliberate, documented with a removal TODO, and pinned against further drift by scripts/targets.test.ts:306-316.

---

Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

[akhileshthite]

@corrideat Left one comment, I do not have much expertise in db, so I have not reviewed the swap itself or the SQL. Someone else should look at that part.

But I read #150, so I can speak for that side: this fixes it properly. The old
failure was @db/sqlite downloading the SQLite library at runtime through
@denosaurs/plug, and the compiled binary having --allow-write=./, so it
could not write ~/Library/Caches/deno/plug and every command died with
"Failed to load SQLite3 Dynamic Library", even chel --version. Embedding the
prebuilt addon per target removes the download completely, so there is nothing
left to fail.

That means I can drop the DENO_SQLITE_PATH workaround in
scripts/chel.mjs in TodoMVC once this is released. Happy to test the released
binary on a clean machine and confirm, since that is exactly how I hit the bug.

[corrideat]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.3

Click to expand review

Review complete. I examined the full diff (including the parts truncated in the provided patch: src/serve/database-sqlite.ts, src/migrate.ts, scripts/targets.ts, and all new test files), verified better-sqlite3@13.0.3's actual lib/binding.js resolution logic and its shipped prebuild manifest against the claims in the code comments, and cross-checked the fingerprint/mtime cache, the same-file guard, the committed bundle, and deno.lock. The core logic is sound; below are the issues found, ordered by importance.

1. 🟡 README's Alpine/musl instructions are internally inconsistent and prescribe an unnecessary native build

• Addressed
• Dismissed

README.md:493-505:

Even from source, musl needs the addon compiled locally, because no prebuild
matches:

```bash
cd node_modules/better-sqlite3 && npm run build-release

Deno also reports the wrong libc to the addon's prebuild lookup
(denoland/deno#33948), so the
freshly built copy may still be skipped. Until that is fixed, patch
node_modules/better-sqlite3/lib/binding.js so that isLinuxMusl always
returns true.

The premise "no prebuild matches" is factually wrong: better-sqlite3@13.0.3 ships `prebuilds/linuxmusl-x64.node` and `prebuilds/linuxmusl-arm64.node` in the published tarball (verified against the package contents), so a prebuild *does* match on musl. What fails is only the *selection*: under the Deno bug, `isLinuxMusl()` returns `false`, so the glibc file is picked instead. Consequently:

- `npm run build-release` (compiling ~10 MB of SQLite C sources, minutes of work) is dead weight: `getPrebuildPath()` in `lib/binding.js` short-circuits before the node-gyp fallback (`build/Release/better_sqlite3.node`) is ever consulted, so with the described patch the *shipped* musl prebuild is selected and the locally built copy is never loaded.
- "the freshly built copy may still be skipped" is therefore backwards — being skipped is exactly the desired outcome once the shipped musl prebuild matches.

Suggested replacement:

```markdown
Even from source, Deno misreports the libc ([denoland/deno#33948]), which makes
the addon's resolver pick the glibc prebuild on musl systems. The musl prebuild
is already shipped inside the package; only the detection is wrong. Until the
Deno bug is fixed, patch `node_modules/better-sqlite3/lib/binding.js` so that
`isLinuxMusl` always returns `true`, which selects the bundled
`prebuilds/linuxmusl-<arch>.node` — no local compilation is needed.

(If a local build genuinely was needed in your testing for a reason not visible from the code, the two sentences still contradict each other and should be reconciled.)

2. 🟡 New scripts/launcher.test.ts duplicates test/bin-chel.test.ts, which also still hand-rolls the temp-dir scaffolding this PR extracted

• Addressed
• Dismissed

scripts/launcher.test.ts:58-97 re-tests three behaviors that test/bin-chel.test.ts already pins, using a second, different scaffolding (fixture node_modules + copied launcher vs. NODE_PATH):

await t.step('exits 127 with an explanation when no sub-package is installed', async () => {
...
await t.step('exits 126 when the sub-package is installed but its binary is missing', async () => {
...
    name: 'exits 126 when the binary exists but is not executable',

vs. test/bin-chel.test.ts:129-173 (reports an unsupported platform…, reports a missing binary…, reports a non-executable binary). Two suites asserting the same documented launcher contract will drift apart when one is updated (e.g. if an exit code or message is reworded, only one suite's copy gets fixed half the time). Consolidate: keep scripts/launcher.test.ts as the single launcher suite (its fixture node_modules is closer to a real install) and move the two cases it does not cover — signal forwarding (128+signum) and the chelBinary-absent fallback name — into it, deleting test/bin-chel.test.ts.

Relatedly, test/bin-chel.test.ts:4-6 and :92 still copy-paste the scaffolding that test/test-helpers.ts was created to deduplicate ("Scaffolding shared by the test suites… previously copy-pasted per file"):

// Ensures ./test/temp exists on a fresh checkout (same pattern as
// sync-versions.test.ts). --allow-write=. in the test task covers it.
await Deno.mkdir('./test/temp', { recursive: true })
...
const tmpDir = await Deno.makeTempDir({ dir: './test/temp' })
try {
  ...
} finally {
  await Deno.remove(tmpDir, { recursive: true })
}

If the file is kept, its six occurrences of that pattern should use withTempDir (which owns the ./test/temp creation), i.e.:

import { withTempDir } from './test-helpers.ts'

Deno.test('bin/chel.js forwards normal exit code from child', async () => {
  if (SKIP_SPAWNING_TESTS) return
  await withTempDir(async (tmpDir) => {
    const name = await currentSubPkgName()
    await setupFakePackage(tmpDir, name, 'process.exit(42)')
    assertEquals(await runShim(tmpDir), 42)
  })
})

3. 🟡 The only test that proves the embedded addon actually loads is not run anywhere automatically

• Addressed
• Dismissed

test/compile-smoke.test.ts:4-9 states it plainly:

// This is the only test that exercises the prebuild mapping in
// scripts/paths.ts for real: everything else compares strings. A wrong mapping
// produces a binary that compiles and starts fine and only fails the moment
// the sqlite backend is used, so it is worth having, but a compile costs
// minutes and ~100 MB of disk. It therefore stays out of `deno task test` and
// runs on request

Everything in deno task test (targets.test.ts, bundle.test.ts) verifies string invariants; the actual --exclude node_modules + --include ./node_modules/better-sqlite3/... interplay that makes import 'npm:better-sqlite3' resolve inside a compiled binary — the riskiest mechanism in this PR — is verified only when a human remembers to run it. A Deno upgrade changing --exclude/--include or snapshot semantics would otherwise surface at release time, or worse, at a user's runtime on a non-host platform. Since CI (.github/workflows/ci.yml) runs on a fixed ubuntu runner, wiring it in is cheap relative to what it protects:

    - name: Run tests
      run: deno task test
    - name: Smoke-test compiled binary (host target)
      run: CHEL_SMOKE_COMPILE=1 deno task smoke

(Optionally as a separate non-blocking job if the ~100 MB disk cost is a concern.)

4. ⚪️ rewordedAddonLoadError advice is wrong for the from-source case, and may miss the most likely musl failure message

• Addressed
• Dismissed

src/serve/database-sqlite.ts:63-69:

  const advice = os === 'linux'
    ? 'The pre-built binaries chel ships for Linux are linked against glibc, ' +
      'so musl-based distributions (Alpine, for example) are not covered; run ' +
      'chel from source with Deno there, or pick a different database backend.'

Two gaps:

1. The function is also reached when running from source with Deno on musl (the README explicitly recommends that path). There, "run chel from source with Deno there" echoes back what the user is already doing. Consider wording that covers both, e.g. "…not covered; run chel from source with Deno and a musl-compatible addon there (see the Supported Platforms section of the README), or pick a different database backend."
2. With the unfixed Deno libc bug (Deno incorrectly reports nonexistent glibc version denoland/deno#33948), on Alpine the compiled binary contains prebuilds/linux-x64.node, so the lookup succeeds and the failure happens later, in dlopen, as a loader error (ld-linux-x86-64.so.2: cannot open shared object file / GLIBC_2.xx not found). Depending on the dlerror text, such a message may name neither better_sqlite3.node nor prebuilds/, so the regex at src/serve/database-sqlite.ts:62 (/better_sqlite3\.node|prebuilds[\\/]/) returns null and the cryptic loader error propagates — for exactly the scenario the rewording was written for. Consider extending the pattern, e.g.:

const ADDON_LOAD_FAILURE = /better_sqlite3\.node|prebuilds[\\/]|ld-linux|GLIBC/i

(Lower confidence since the exact dlerror wording varies by musl version and Deno's error wrapping.)

5. ⚪️ isSameSqliteFile is production-exported but only consumed by tests

• Addressed
• Dismissed

src/migrate.ts:94-101:

export function isSameSqliteFile (
  fromBackend: string | undefined,
  toBackend: string | undefined,
  fromOptions: BackendOptionsLike,
  toOptions: BackendOptionsLike
): boolean {
  return sharedSqliteFilepath(fromBackend, fromBackend === undefined ? toBackend : toBackend, fromOptions, toOptions) !== null
}

migrate() itself only calls sharedSqliteFilepath (src/migrate.ts:146); the boolean wrapper exists solely so migrate.test.ts / migrate-symlinks.test.ts read nicely. Either have the tests assert sharedSqliteFilepath(...) !== null and delete the wrapper, or keep it but note in a comment that it is a test-facing convenience so it isn't mistaken for part of the guard's production surface.

6. ⚪️ Naming: rewordedAddonLoadError reads as a past participle, and its return value is an advice-bearing error, not a "reworded" one

• Addressed
• Dismissed

src/serve/database-sqlite.ts:57-60:

export function rewordedAddonLoadError (
  cause: unknown,
  os: typeof Deno.build.os = Deno.build.os
): Error | null {

Every call site has to read it twice ("reworded… error" — whose error?). Something verb- or role-shaped matches its actual job (turn an addon-load failure into an actionable error, or null):

export function addonLoadAdvice (cause: unknown, os: typeof Deno.build.os = Deno.build.os): Error | null
// or
export function explainAddonLoadFailure (cause: unknown, os: typeof Deno.build.os = Deno.build.os): Error | null

7. ⚪️ binaries.ts now carries two parallel tree walks with subtly different cycle-tracking semantics

• Addressed
• Dismissed

scripts/binaries.ts:122-147 (normalizePathMtimes) and :188-224 (digestsAt/pathDigests) both implement the same traversal (realPath → seen guard → NotFound-tolerant recursion), but with asymmetric rules: normalizePathMtimes adds every real path, files included, to seen (:134-135), while digestsAt only adds directories (:211-212), so a file reachable twice is stamped once but hashed twice (under two logical keys). Both behaviors are defensible in isolation, but the asymmetry is undocumented and invites a future "fix" in either direction. A single shared walker parameterized by a per-file callback would remove the duplication and make the one intentional difference explicit:

async function walkFiles (
  dir: string,
  seen: Set<string>,
  onFile: (path: string) => Promise<void>
): Promise<void>

with normalizeMtimes passing (p) => Deno.utime(p, time, time) and fileDigests collecting digests. Low urgency — the current code is correct and well-commented.

8. ⚪️ Drive-by: AGENTS.md still says "Three persistence backends" while listing four (plus the router this PR touches)

• Addressed
• Dismissed

AGENTS.md:200 (in the "Database Backends" section, adjacent to sections this PR edits):

Three persistence backends available:
- `mem` - In-memory (default in development)
- `fs` - Filesystem
- `sqlite` - SQLite database
- `redis` - Redis server

Four are listed and router (which this PR's migrate guard explicitly reasons about) is missing. Since this PR makes sqlite a first-class citizen, worth fixing while in here: "Five backends available: mem, fs, sqlite, redis, router".

---

Not issues, for the record (verified rather than assumed): the "all eight prebuilds / 14.8 MB saved" figures match the actual better-sqlite3@13.0.3 contents; the prebuild(os, cpu) mapping mirrors the real lib/binding.js resolution; the same-file guard correctly handles case-insensitive filesystems, symlinks, omitted filepaths, and router entries on both sides; deno.lock is coherent after removing the @denosaurs/plug chain; no @db/sqlite references remain anywhere; and all new *.test.ts files are committed, satisfying the new tracked-tests guard.

---

Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

[corrideat]

LGTM

[taoeffect]

Let me know @corrideat when you've tested this with Group Income and I can merge (if all looks good with the PR)