fix(install): build native addons on Homebrew installs - #120
Merged
Conversation
A fresh `brew install jinn` produced a gateway that could not start, and
after that crash was patched, one where no session could ever spawn. Both
trace to a single line in the formula.
Homebrew's `std_npm_args` folds in `npm_install_security_args`, whose
`ignore_scripts:` defaults to true, so `system "npm", "install",
*std_npm_args` installs the whole tree with `--ignore-scripts`:
* better-sqlite3's `install` (prebuild-install || node-gyp rebuild)
never runs, so no compiled addon is produced at all and the gateway
dies at boot with "Could not locate the bindings file" (#108).
* node-pty's `install` never runs either, so it falls back to its
checked-in prebuilds, whose `spawn-helper` ships at mode 0644. macOS
posix_spawns that helper on every pty, so every session dies with
"posix_spawnp failed." (#109). jinn-cli's own postinstall exists to
restore that bit and is skipped for the same reason.
Enabling scripts alone was not sufficient. `std_npm_args` also passes
`--build-from-source`, under which node-pty compiles and DELETES
`prebuilds/` entirely, leaving only `build/Release`. The old postinstall
did `readdir("prebuilds")` and threw ENOENT on exactly that layout, so
the one-line formula change turned a runtime crash into a hard
`brew install` failure. Verified by installing the published tarball with
those flags: npm exits 1.
Changes:
* Formula: `std_npm_args(ignore_scripts: false)`, so the compile that
`depends_on "python" => :build` and `--build-from-source` already
imply actually happens.
* postinstall: cover every layout node-pty can resolve from
(build/Release, build/Debug, prebuilds/<platform>-<arch>) and never
fail an install over a permission fix-up.
* runtime guard: open a database instead of only requiring
better-sqlite3. The binding resolves lazily in the Database
constructor, so `require('better-sqlite3')` SUCCEEDS on a
binding-less install — the guard and the formula's own test block
were both false negatives and could never have caught #108.
A missing binding now reports as missing rather than as a Node
version mismatch it is not.
* runtime guard: repair the spawn-helper exec bit at startup. This is
the only layer that helps an install we never got to run scripts in,
including the already-broken ones on disk today.
* formula test block: open a database and really spawn through
node-pty, since both bugs survive a bare require. Drop the
`require('classic-level')` assertion — classic-level is a root
workspace dep, is not published inside jinn-cli, and that line could
only ever fail once someone ran `brew test`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks to @yoobi for both reports — they were precise, correctly identified the shared cause in the formula, and correctly called out that #109 was masked by #108. Diagnosis confirmed on a reproduction of the published 0.29.0 tarball.
Root causes
Both issues come from one line. Homebrew's
std_npm_argsfolds inLanguage::Node.npm_install_security_args, whoseignore_scripts:parameter defaults totrue, so the formula installed the whole tree with--ignore-scripts.#108 — better-sqlite3 binding never built. better-sqlite3 declares
"install": "prebuild-install || node-gyp rebuild --release". With scripts suppressed it never runs, and the package has nobuild/directory at all. The gateway dies atinitDbwithCould not locate the bindings file.#109 — node-pty
spawn-helpernot executable. With scripts suppressed, node-pty falls back to its checked-in prebuilds, whosespawn-helperships at mode0644. On macOSunixTerminal.jsposix_spawns that helper on every pty, so every session dies withposix_spawnp failed..jinn-cli's ownpostinstallexists to restore that bit and is skipped for exactly the same reason.A third cause, which the one-line formula fix would have hit
Enabling scripts alone is not sufficient, and on its own makes things worse.
std_npm_argsalso passes--build-from-source. Under that flag node-pty compiles and deletesprebuilds/entirely, leaving onlybuild/Release/{pty.node,spawn-helper}. The previousscripts/fix-node-pty-permissions.mjsdidreaddir(nodePtyRoot + "/prebuilds")and threw on ENOENT, which failsnpm installand would have turned a runtime crash into a hardbrew installfailure.Verified against the published tarball:
Why this shipped undetected
require('better-sqlite3')succeeds on a binding-less install — the binding is resolved lazily inside theDatabaseconstructor, not at module load. Measured on the broken tree:So two things that looked like they were guarding this were not:
assertNativeRuntime()inbin/jinn.tsanddaemon-entry.tsonly did a barerequire, so it passed and let the rawbindingsstack trace escape from deep inside boot.test doblock assertedrequire('better-sqlite3'). Contrary to the hope in Homebrew install:jinn startcrashes because thebetter-sqlite3native binding is never built #108, that line would not have caught this bug even ifbrew testhad run. The same is true ofrequire('node-pty'), which passes fine whilespawn-helperis0644.That block also asserted
require('classic-level'), which is a root workspace dependency and is not published insidejinn-cli, so it could only ever fail once someone actually ranbrew test.What changed
Formula/jinn.rb—system "npm", "install", *std_npm_args(ignore_scripts: false). This makes the compile thatdepends_on "python" => :buildand--build-from-sourcealready imply actually happen.packages/jinn/scripts/fix-node-pty-permissions.mjs— covers every layout node-pty resolves from (build/Release,build/Debug,prebuilds/<platform>-<arch>) instead of assumingprebuilds/, and never fails an install over a permission fix-up. It warns and defers to the runtime repair instead.packages/jinn/src/shared/runtime-guard.ts—assertNativeRuntime()now opens a:memory:database rather than only requiring the module, and distinguishesbinding-missingfromabi-mismatchso a never-built addon stops being reported as a Node version mismatch it is not. NewrepairNodePtySpawnHelper()restores thespawn-helperexec bit at startup; wired into bothbin/jinn.tsanddaemon-entry.ts.test do— opens a database and really spawns through node-pty, since both bugs survive a barerequire. Drops theclassic-levelassertion.Why a runtime self-heal as well as the formula fix
The formula fix is the real root-cause fix and is what closes both issues for new installs. The startup repair is kept alongside it for the part packaging cannot reach:
posix_spawnp failed.becausenode-pty'sspawn-helperlacks the executable bit #109 today gets it back on the nextjinn start, without waiting for a formula bump to propagate.npm ci --ignore-scripts, corporate npm configs, sandboxed CI).chmodon a file whose path we derive the same way node-pty does — cheap, idempotent, and a no-op on a healthy install.The equivalent was deliberately not done for better-sqlite3: fabricating a native addon at runtime means shelling out to
node-gyp/prebuild-install, which needs network and a compiler toolchain and takes minutes. Doing that silently from a daemon start is worse than failing. Instead the guard now detects that exact state and prints the precise command to run. Shipping prebuilt binaries in the tarball was also rejected — better-sqlite3 is a transitive dependency, not ours to vendor, and it would multiply the package size per platform.Verification
Reproduced and fixed against the real published
jinn-cli-0.29.0tarball in a scratch prefix, using Homebrew's exact flags.Broken (current formula,
--ignore-scripts) — both symptoms reproduce:Startup repair, applied to that same broken tree:
Fixed formula flags + this branch's package (
npm install --global --build-from-source, i.e.std_npm_args(ignore_scripts: false)):Regression coverage. Three new suites, all of which fail without this change (13 of 15 failed on the pre-fix source; the postinstall suite fails 3 of 5 against the old script):
src/shared/__tests__/runtime-guard.test.ts— failure classification and the spawn-helper repair across all three node-pty layouts.src/shared/__tests__/postinstall-node-pty.test.ts— runs the real postinstall as npm would, including the source-built layout with noprebuilds/that previously threw.src/shared/__tests__/homebrew-formula.test.ts— locks the formula'signore_scripts: false, the non-false-negative test assertions, and that the test block only requires modules the published package actually depends on.ruby -c Formula/jinn.rb→ Syntax OK.pnpm typecheckat root → 2 successful. Full suite frompackages/jinn:No flakes tripped; no re-runs needed.
Note on release gating
brew teststill never runs duringbrew install, so the hardened test block only protects us if releases are gated on it. That CI job is not added here — worth a follow-up.Fixes #108
Fixes #109