fix(desktop): verify better-sqlite3's shipped prebuild - #485
Conversation
Greptile SummaryThe PR updates desktop artifact verification for better-sqlite3 v13’s shipped N-API prebuild layout instead of expecting the legacy rebuilt binding.
Confidence Score: 5/5The PR appears safe to merge, with the verifier aligned to the shipped better-sqlite3 v13 prebuilds on every supported desktop target. The supported platform and architecture mappings resolve to the documented v13 prebuild filenames, unsupported platforms are rejected before verification, and the retained native-header checks continue to detect architecture mismatches.
|
| Filename | Overview |
|---|---|
| apps/desktop/scripts/verify-artifacts.mts | Updates better-sqlite3 verification to inspect the target-specific v13 N-API prebuild while preserving binary architecture checks. |
| apps/desktop/electron-builder.yml | Corrects comments to reflect that packaged applications load better-sqlite3’s shipped prebuild rather than a rebuilt Release binding. |
| apps/daemon/scripts/package-daemon.mts | Updates packaging documentation to distinguish better-sqlite3’s target prebuilds from host-specific native dependencies. |
| apps/daemon/AGENTS.md | Aligns daemon packaging guidance with the current native dependency distribution model. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Target platform and architecture] --> B[Map builder platform to Node platform]
B --> C[Resolve better-sqlite3 prebuild path]
C --> D[Check app.asar.unpacked]
D --> E[Validate Mach-O, PE, or ELF architecture]
E --> F[Accept or reject artifact]
Reviews (1): Last reviewed commit: "fix(desktop): verify better-sqlite3's sh..." | Re-trigger Greptile
There was a problem hiding this comment.
ℹ️ The fix itself is correct and I could not find a way for it to fail. Two comment-accuracy points inline, plus two more stale sites the sweep missed.
Reviewed changes — the whole 1-commit diff (4 files), plus independent verification of the better-sqlite3 v13 layout, its binding-resolution order, and electron-builder's smart-unpack behavior against the installed packages.
verify-artifacts.mtsasserts the shipped prebuild —NATIVE_BINDINGis replaced bysqliteBinding(platform, arch)over a newNODE_PLATFORMmap, targetingnode_modules/better-sqlite3/prebuilds/<nodePlatform>-<arch>.node; the Mach-O/PE/ELF arch check is retained and the entry becomes conditional, mirroringkeyringBinding.- Three comment corrections —
electron-builder.yml,apps/daemon/scripts/package-daemon.mts, andapps/daemon/AGENTS.mddrop theirprebuild-install/build/Releaseclaims.
What I verified independently, since the whole PR rests on it:
better-sqlite3@13.0.2ships eight flatprebuilds/<platform>-<arch>.nodefiles,gypfile: false, no install script, andbinding.gypsets both targets totype: nonewhen a prebuild exists — so@electron/rebuildreally is a no-op andbuild/holds only node-gyp stamps.lib/binding.js'sgetPrebuildPath()is tried beforebuild/Release, so the asserted file is genuinely the one the daemon loads.app-builder-lib/out/asar/unpackDetector.jsaddsstat.moduleRootPathtoautoUnpackDirson any.nodehit, so the wholebetter-sqlite3directory is smart-unpacked — the asserted path is correct for all six mac/win/linux × x64/arm64 targets, not just the arm64 mac the description covers.- Nothing in
.github/workflows/, the packaging scripts, or the E2E suite still referencesbuild/Release/better_sqlite3.node.
ℹ️ The stale-comment sweep stops two sites short
The PR corrects three comments but leaves two more carrying the same pre-v13 premise, one of which now directly contradicts an edit in this diff.
Technical details
# Two more `@electron/rebuild` / `build/Release` claims left stale
## Affected sites
- `docs/RELEASE.md:257` — describes this exact gate as "the smartUnpacked `better-sqlite3` binding is
present and its Mach-O/PE/ELF header targets the app's arch (**the tripwire for the CODE-107
Windows rebuild miss** …)". There is no rebuild left to miss: `binding.gyp` short-circuits to
`type: none` and the prebuild is named by arch, so the header check can no longer fail for that
reason. The sentence should describe what the gate now covers (the prebuild for the target is
present and smart-unpacked) rather than a tripwire that no longer exists.
- `apps/desktop/scripts/package-app.mts:116` — `pruneStaging`'s doc says "notably
better-sqlite3/deps must stay HERE in staging because `@electron/rebuild` compiles from it before
collection", presenting the from-source compile as the routine path. The `electron-builder.yml`
edit in this diff (new lines 61-63) now says `deps` is "only ever compiled if a from-source
`@electron/rebuild` happens". The two now disagree about the same fact.
## Required outcome
- Both sites state the post-v13 reality, and `electron-builder.yml` and `package-app.mts` agree on
whether the `better-sqlite3/deps` compile is routine or conditional.
## Open questions for the human
- Given the compile is now unreachable in practice, is keeping `better-sqlite3/deps` in staging
still worth stating as a constraint, or has it become a plain "harmless, excluded from the asar
anyway"? That answer decides how both comments should read.ℹ️ Nitpicks
NODE_PLATFORM(verify-artifacts.mts:79-83) has to stay key-for-key withEXPECTED; a fourth platform added toEXPECTEDwithout a matching entry makessqliteBindingreturnnulland the better-sqlite3 gate disappear silently instead of failing.Partial<Record<string, string>>gives no help here. Not worth a runtime guard — but the two maps being coupled is invisible from either one.
Claude Opus | 𝕏
| * this targets plain Node. better-sqlite3 carries a NAPI prebuild per target, but the host-only | ||
| * napi-rs optional deps and @sentry's ABI-pinned profiler do not: a same-platform artifact — build | ||
| * it on (or for) each target. |
There was a problem hiding this comment.
The @sentry half of this rationale doesn't hold. @sentry/node-cpu-profiler (pulled in by @sentry/profiling-node) ships 35 binaries in its own tarball — lib/sentry_cpu_profiler-{darwin,linux,win32}-{x64,arm64}[-glibc|-musl]-{108,115,127,137,147}.node — so it very much does carry a prebuild per target; it has no install-time platform scoping, and check-build.js prunes nothing. What it is pinned to is the Node module ABI, which is a same-Node-version constraint, not a same-platform one. The conclusion is still right, but it rests entirely on @napi-rs/keyring, whose optional deps really do install host-triple-only (only @napi-rs/keyring-linux-x64-gnu is present in this checkout). Worth naming the one true reason rather than two, since a future reader deciding whether the standalone daemon can be cross-built will act on this sentence. (If you want a second genuine platform constraint, the profiler ships no win32-arm64 binary at all.)
| binary — a **same-platform** artifact, build per target), and it prunes the host-arch agent CLI | ||
| platform packages (the daemon downloads them at runtime via `@linkcode/assets`, as the desktop | ||
| distinct from the desktop bundle: it targets **plain Node** (still a **same-platform** artifact — the | ||
| napi-rs optional deps and @sentry's profiler resolve host-only — build per target), and it prunes |
There was a problem hiding this comment.
Same correction as package-daemon.mts — "@sentry's profiler resolve[s] host-only" is the clause that's wrong: it ships every platform × ABI in its tarball. @napi-rs/keyring is what makes this a same-platform artifact.
| * better-sqlite3's binding, smartUnpacked beside the asar; the daemon requires it at boot. Since | ||
| * v13 it is one NAPI prebuild per platform-arch shipped in the tarball (`build/Release` is only | ||
| * written when node-gyp actually compiles, which it now skips), so what breaks is the staging | ||
| * prune keeping the wrong target — every client then shows "Unable to connect to the daemon". |
There was a problem hiding this comment.
"what breaks is the staging prune keeping the wrong target" isn't reachable in this repo yet: pruneStaging in package-app.mts:118 only deletes .map/.ts/.md files and never touches prebuilds/, and the files globs exclude only better-sqlite3/deps/** — so all eight prebuilds ship in every artifact today. That makes this gate an existence/smart-unpack check, with the arch-header assertion tautological (the file is named by arch and ships verbatim from the tarball). Worth saying that plainly instead, since the sentence as written describes the state after the prune you deferred to a follow-up.

The v0.27.0 release build failed on all three platforms with:
better-sqlite3 13 (#443) changed how the binding is distributed. v12 downloaded a per-Node-ABI
build into
build/Release/better_sqlite3.nodeviaprebuild-install; v13 ships eight N-API(
NAPI_VERSION=10) prebuilds in the tarball asprebuilds/<platform>-<arch>.node, and itsbinding.gypturns both targets intotype: nonewhenever a prebuild exists. node-gyp thereforeonly touches its stamps,
@electron/rebuildis a no-op, and nothing is ever written tobuild/Release— the packaged app is fine,verify-artifacts.mtswas just looking in the oldplace. Every other check in that run passed.
verify-artifacts.mtsnow asserts the target's prebuild instead, keeping the Mach-O/PE/ELF archcheck. Three stale comments about
prebuild-install/build/Releaseare corrected with it.Verification
@electron/rebuildstays a no-op andapp.asar.unpacked/node_modules/better-sqlite3/prebuilds/darwin-arm64.nodeis in place.darwin-x64.nodewhen pointed at an arm64 app as if it were the x64 one.daemon.db, so the shipped prebuild iswhat it loads.
pnpm check:cipasses.Follow-up (not in this PR)
v13's tarball carries all eight prebuilds and electron-builder smart-unpacks the whole module, so
every artifact now ships ~15 MB of foreign-platform
.nodefiles. Pruning the staging dir down tothe target (plus the host's, which is what keeps
binding.gypfrom compiling from source) removesthat; left out here to keep the release unblocking minimal.