fix(build): allow better-sqlite3 and onnxruntime-node install scripts - #167
Conversation
npm 12 blocks dependency install/postinstall scripts by default unless the root package's allowScripts field permits them. Without an entry here, a fresh `npm install` under npm 12 (or npm 11.16+ with blocking opted in) exits 0 while skipping better-sqlite3's node-gyp build and onnxruntime-node's postinstall — so both native bindings never get built. Archiving keeps working, but indexing and search silently never run again; the only trace is 'Could not locate the bindings file' in the plugin log, which nothing surfaces to the user. allowScripts uses bare package names rather than pinned versions, since a pinned key (e.g. "better-sqlite3@12.11.1") stops matching the moment the dependency bumps, which would silently reintroduce this bug on the next release. Fixes obra#162.
obra
left a comment
There was a problem hiding this comment.
Reviewed at d01b622f against base 28f0933. This is right, and the reasoning behind the bare-name choice is right for a reason worth recording. One suggestion below that would make it stronger; not a blocker.
Verified, not assumed
I checked allowScripts against npm's own docs on this machine (npm 12.0.2, npm help install-scripts) rather than trusting the field name — inventing a plausible-looking config key would produce a PR that reads as a fix and does nothing, which is the exact failure mode #162 is about:
Dependency install scripts are blocked by default. Install commands silently skip lifecycle scripts for any dependency that does not have a matching entry in
allowScripts…
Your bare-name decision is explicitly the documented behavior, not a guess:
By default it writes pinned entries (pkg@1.2.3)… Pass
--no-allow-scripts-pinto write name-only entries that allow any future version.
So a name-only key survives a dependency bump, and the pinned form is what silently falls out of the allowlist. The test that rejects /@\d/ keys pins that decision in place. Good.
Mutation-tested both assertions — neither is decorative:
- delete
allowScripts→ fails - change
better-sqlite3tobetter-sqlite3@12.10.0→ fails
On the two-of-five question
npm install-scripts ls here reports 7 blocked entries across 5 unique packages: better-sqlite3, onnxruntime-node, esbuild (×3), protobufjs, sharp. You allowed two, so I checked whether that was a subset or a decision. It's a decision, and I agree with it:
better-sqlite3— runtimedependencies. Essential.onnxruntime-node— what embeddings actually load. Essential.esbuild—devDependencies, used bynpm run bundle. Verified it works blocked:./node_modules/.bin/esbuild --version→0.25.12, exit 0. Modern esbuild ships platform binaries as optional deps, so the postinstall is a fallback. No action needed.protobufjs— transitive underonnxruntime-web, not-node.sharp— transitive under@huggingface/transformers.
Your test title says "the native-binding installs indexing depends on", and the body checks exactly that. Name and body agree, which is worth calling out because the opposite — a title claiming a repo-wide invariant over a hardcoded subset — is a defect I've hit six times in these repos this week.
The one suggestion: deny sharp explicitly
sharp being blocked is not incidental — it's protective. #102 is precisely sharp@0.34.5's postinstall detecting a host libvips, deliberately process.exit(1)-ing to force a source build, and leaving a corrupt node_modules behind. While its scripts are blocked, that can't happen.
But that protection is currently silence, and silence loses to one command. The docs say approve --all "approves every package with unreviewed install scripts in one go" — and sharp is unreviewed. A maintainer hitting a different blocked-script problem runs --all and reintroduces #102 without ever deciding to.
An explicit denial is documented to survive exactly that:
denyrecords an explicit denial for the named packages (a name-onlyfalseentry), which survivesnpm install-scripts approve --alland excludes the package from any future blanket approval.
So:
"allowScripts": {
"better-sqlite3": true,
"onnxruntime-node": true,
"sharp": false
}with a comment in the test explaining that false is load-bearing and points at #102. That turns "we happened not to allow it" into "we decided against it, and the decision is enforced." Worth considering for protobufjs too, though I have no concrete failure to point at there, so I'd leave it unless you do.
Scope of what this proves
Worth stating in the PR body so nobody over-reads it: the test asserts the config, not the behavior. It guarantees the entries don't get deleted or pinned; it does not demonstrate that a fresh npm install under npm 12 now builds the bindings. That would need a clean-install run on npm 12, which is the actual proof #162 wants. Not asking you to build that here — just don't let the green test imply it.
Approving once you take it out of draft. If you add the sharp: false entry I'll re-check and land it; if you'd rather not, say why and I'll land it as-is — my case for it is real but it isn't a blocker.
sharp's postinstall exits 1 on any host with libvips already installed globally and corrupts node_modules on the way out (obra#102). It has been blocked so far only because it's absent from allowScripts — incidental silence, not a decision. `npm install-scripts approve --all` would sweep it into the allowlist on its next run, since an omitted key reads as 'not yet reviewed' rather than 'reviewed and refused'. Set it to `false` explicitly instead: allowScripts is documented to treat an explicit `false` as surviving --all, so this converts 'happened not to allow it' into an enforced decision.
obra
left a comment
There was a problem hiding this comment.
Approving at 10594111. Both new assertions mutation-tested, and I verified the behavior — not just the config — in a clean room, because a config test can't prove npm honors what it says.
Mutation results (2/2 pass at head):
| Mutation | Result |
|---|---|
omit sharp entirely (the weaker form this test exists to forbid) |
fails |
flip sharp to true (reintroduces #102) |
fails |
delete allowScripts |
fails |
pin a key to pkg@version |
fails |
Behavioral verification. I said in my last review that the config test doesn't demonstrate npm actually honors this. So I built a throwaway project with a real npm install and checked both claims directly:
- Bare names are honored.
allowScripts: {"esbuild": true}→npm install-scripts lsreports "No packages with unreviewed install scripts." Your name-only choice works. falsesurvivesapprove --all— the whole point of thesharpentry. Set{"esbuild": false}, rannpm install-scripts approve --all, and the entry came back stillfalse. The blanket approval skipped it exactly as documented.
So the sharp: false line does the job it's there for: #102 can no longer be reintroduced by a maintainer running --all for unrelated reasons.
One correction I owe you, about my own testing. My first behavioral attempt ran npm install-scripts ls inside a git worktree with a symlinked node_modules, and it reported better-sqlite3, onnxruntime-node and sharp as "not covered by allowScripts" — which reads exactly like this PR not working. It also listed 15 packages where the real checkout lists 7.
Before writing that up I probed it: set esbuild: true in the worktree's package.json and re-ran. esbuild stayed in the blocked list, which means npm was never reading that package.json — it resolved the project root back through the symlink to the real checkout. My harness was broken, not your change.
Recording it because the near-miss is the interesting part: I had a plausible, specific, reproducible "finding" that was entirely an artifact of how I set the test up. The thing that caught it was asking whether my probe could distinguish the two explanations before reporting either. Same failure shape as reading a stale ref, or grepping for an interface that's declared as a class.
CI green on Node 22 and 24. Landing this.
…obra#169) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hi4Rc7k1vn1mSGxPv4NvbA
Summary
Under npm 12 (and npm 11.16+ with script blocking opted in), a fresh
npm installof the plugin skipsbetter-sqlite3's native build andonnxruntime-node's postinstall entirely, unless the root package'sallowScriptsexplicitly permits them.npm installstill exits 0 andprints "rebuilt dependencies successfully" — it just doesn't build
anything. Archiving keeps working, but indexing and search silently
never run again. The only trace is:
in the plugin log, which nothing surfaces to the user.
Root cause
Neither
better-sqlite3(needs a node-gyp/prebuild-install step) noronnxruntime-node(needs its postinstall to fetch the runtime) islisted in
allowScripts, so npm 12's default script-gating blocks bothon a clean install.
Fix
Add
allowScriptstopackage.jsonfor the two packages indexingdepends on, keyed by bare package name (not a pinned version) so a
future dependency bump doesn't silently fall out of the allowlist and
reintroduce this bug:
Also explicitly deny
sharp's install script rather than leaving itabsent.
sharp's postinstall exits 1 on any host that already haslibvips installed globally, and corrupts
node_moduleson the way out(#102). It's been blocked so far only because it's missing from
allowScripts— incidental silence, not a decision — andnpm install-scripts approve --allwould sweep an absent key into theallowlist on its next run, since "absent" reads as "not yet reviewed"
rather than "reviewed and refused." An explicit
falseis documentedto survive
--all, converting "we happened not to allow it" into anenforced decision.
Test plan
test/allow-scripts.test.ts: readspackage.jsonand assertsallowScripts.better-sqlite3andallowScripts.onnxruntime-nodearetrue, thatallowScripts.sharpis present and explicitly
false(not merely absent), and that nokey is a pinned
name@version(which would stop matching on thenext bump). Fails on
main(allowScriptsundefined /sharpkeymissing), passes with this change.
npx vitest run test/allow-scripts.test.ts— 2/2 passnpm test(full suite) — 350/350 pass, 63 test files, noregressions
dist/changes needed —allowScriptsis npm-level installconfig, not bundled source.
Scope note: this test suite asserts the
allowScriptsconfig ispresent and correctly shaped (right packages, right booleans, bare
names). It does not — and can't, from this environment — prove that a
fresh install under an actual npm 12 host now builds the native
bindings end-to-end. That still wants a maintainer confirming on a real
npm 12 install per the issue's repro steps.
Fixes #162.