Skip to content

fix(build): allow better-sqlite3 and onnxruntime-node install scripts - #167

Merged
obra merged 2 commits into
obra:mainfrom
ada-sen:fix/162-npm12-allowscripts
Sep 10, 2026
Merged

obra merged 2 commits into
obra:mainfrom
ada-sen:fix/162-npm12-allowscripts

Conversation

@ada-sen

@ada-sen ada-sen commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Under npm 12 (and npm 11.16+ with script blocking opted in), a fresh
npm install of the plugin skips better-sqlite3's native build and
onnxruntime-node's postinstall entirely, unless the root package's
allowScripts explicitly permits them. npm install still exits 0 and
prints "rebuilt dependencies successfully" — it just doesn't build
anything. Archiving keeps working, but indexing and search silently
never run again. The only trace is:

Error syncing: Error: Could not locate the bindings file

in the plugin log, which nothing surfaces to the user.

Root cause

Neither better-sqlite3 (needs a node-gyp/prebuild-install step) nor
onnxruntime-node (needs its postinstall to fetch the runtime) is
listed in allowScripts, so npm 12's default script-gating blocks both
on a clean install.

Fix

Add allowScripts to package.json for the two packages indexing
depends 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:

"allowScripts": {
  "better-sqlite3": true,
  "onnxruntime-node": true,
  "sharp": false
}

Also explicitly deny sharp's install script rather than leaving it
absent. sharp's postinstall exits 1 on any host that already has
libvips installed globally, and corrupts node_modules on the way out
(#102). It's been blocked so far only because it's missing from
allowScripts — incidental silence, not a decision — and npm install-scripts approve --all would sweep an absent key into the
allowlist on its next run, since "absent" reads as "not yet reviewed"
rather than "reviewed and refused." An explicit false is documented
to survive --all, converting "we happened not to allow it" into an
enforced decision.

Test plan

  • New regression test in test/allow-scripts.test.ts: reads
    package.json and asserts allowScripts.better-sqlite3 and
    allowScripts.onnxruntime-node are true, that allowScripts.sharp
    is present and explicitly false (not merely absent), and that no
    key is a pinned name@version (which would stop matching on the
    next bump). Fails on main (allowScripts undefined / sharp key
    missing), passes with this change.
  • npx vitest run test/allow-scripts.test.ts — 2/2 pass
  • npm test (full suite) — 350/350 pass, 63 test files, no
    regressions
  • No dist/ changes needed — allowScripts is npm-level install
    config, not bundled source.

Scope note: this test suite asserts the allowScripts config is
present 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.

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 obra left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-pin to 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-sqlite3 to better-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 — runtime dependencies. Essential.
  • onnxruntime-node — what embeddings actually load. Essential.
  • esbuild — devDependencies, used by npm 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 under onnxruntime-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:

deny records an explicit denial for the named packages (a name-only false entry), which survives npm install-scripts approve --all and 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.
@ada-sen
ada-sen marked this pull request as ready for review September 10, 2026 16:25

@obra obra left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ls reports "No packages with unreviewed install scripts." Your name-only choice works.
  • false survives approve --all — the whole point of the sharp entry. Set {"esbuild": false}, ran npm install-scripts approve --all, and the entry came back still false. 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
obra merged commit cd8dc33 into obra:main Sep 10, 2026
2 checks passed
vicnaum added a commit to vicnaum/episodic-memory that referenced this pull request Sep 24, 2026
…obra#169)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hi4Rc7k1vn1mSGxPv4NvbA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

npm 12 blocks better-sqlite3 and onnxruntime-node install scripts; indexing silently never works

2 participants