Skip to content

fix(build): fail fast when an externalised optional native dep was silently dropped - #11863

Merged
diegosouzapw merged 6 commits into
cheaper-inference:release/v3.8.51from
ujjawalkaushik1110:fix/native-dep-preflight
Aug 30, 2026
Merged

diegosouzapw merged 6 commits into
cheaper-inference:release/v3.8.51from
ujjawalkaushik1110:fix/native-dep-preflight

Conversation

@ujjawalkaushik1110

Copy link
Copy Markdown
Contributor

Summary

  • npm run build now runs a one-second preflight (npm run check:native-deps) that names any package which is both listed in serverExternalPackages and declared in optionalDependencies but absent from the tree, and prints the exact install command to fix it.
  • Fixes a ReferenceError in scripts/build/postinstall.mjs that turned a failed better-sqlite3 rebuild into a failed npm install.

The failure this replaces

npm 11 (bundled with Node 24+) will not run install scripts for optional dependencies unless they are approved. better-sqlite3 is an optionalDependency whose install script builds a native addon, so npm skips it, removes it from the tree, and exits 0. The install output never says the package is gone.

The build then fails roughly four minutes later:

Error: Module not found: Can't resolve 'better-sqlite3'
  ./open-sse/mcp-server/audit.ts:227:11
  ./src/lib/db/adapters/runtimeRequire.ts:23:16

Being listed in serverExternalPackages does not save it — the bundler has to resolve a request before it can externalise it, so an absent package is a hard build failure. The message reads like a code bug, and both import traces point at application source rather than at the install, which is what makes it expensive to diagnose. It is already documented at docs/guides/TROUBLESHOOTING.md#npm-v11-better-sqlite3-not-installed-cannot-find-module, but only for the runtime symptom (Cannot find module), not the build one.

The gate fails only where the build would have failed anyway. It just costs one second instead of four minutes, and it names the package and the fix.

The postinstall ReferenceError

fixBetterSqliteBinary() falls back to npm rebuild better-sqlite3 under a timeout. Its catch block reports which limit was used (600s on Termux/Android, 300s otherwise) by reading isAndroid — but isAndroid was declared with const inside the try, so it was not in scope in the catch.

postinstall.mjs is a top-level-await module, so the ReferenceError escaped the catch, rejected module evaluation, and failed the entire npm install — on the exact path whose purpose is to print manual-fix instructions for machines without a C++ toolchain. Users got a stack trace instead of the guidance. The declaration is hoisted above the try; nothing else changes.

Related Issues

Validation

  • Change type: build-deploy
  • Focused tests and category gates from the golden path
  • Production-code changes include a new or updated automated test in this PR

Focused loop:

node --import tsx/esm --test tests/unit/build/check-native-deps.test.ts
node --import tsx/esm --test tests/unit/build/postinstall-rebuild-timeout-scope.test.ts
npm run check:native-deps      # green tree: exits 0
npm run lint

Manual reproduction of the gate:

mv node_modules/better-sqlite3 /tmp/bs3
npm run check:native-deps      # exits 1, names better-sqlite3, prints the fix
mv /tmp/bs3 node_modules/better-sqlite3
npm run check:native-deps      # exits 0

The originating failure was reproduced on Windows 11 + Node 24 + npm 11, building v3.8.51 from source: npm install exited 0 with no node_modules/better-sqlite3, and npm run build then failed with the two Module not found errors quoted above.

Tests Added Or Updated

  • tests/unit/build/check-native-deps.test.ts — new. Covers extractServerExternalPackages (ordering, comment lines, single quotes, unterminated arrays, and the case where a prose mention of serverExternalPackages in a comment shadowed the real declaration), findMissingExternals (optional-only scoping, order preservation, injected resolver), and formatMissingReport (empty case, package names, runnable command, troubleshooting link). One test asserts against the real next.config.mjs so the gate cannot silently stop seeing better-sqlite3.
  • tests/unit/build/postinstall-rebuild-timeout-scope.test.ts — new. Source-level regression guard: isAndroid is declared exactly once and before the try, the catch still derives the reported timeout from it, and the manual-fix guidance is still reachable. Reads the source rather than importing it, because postinstall.mjs does its work at module scope and importing it would run the real postinstall as a test side effect.

Coverage Notes

  • scripts/ is outside the src/ / open-sse/ / electron/ / bin/ coverage scope. The three exported functions in check-native-deps.mjs are pure and covered by the new unit test; main() is the thin I/O wrapper.
  • No coverage moved down in any touched file.

Reviewer Notes

  • The new prebuild hook is the only behavioural risk. npm runs it for npm run build but not for build:secure or build:backend, which invoke build-next-isolated.mjs directly. If any pipeline builds with optional dependencies deliberately absent, this would now fail it — set OMNIROUTE_SKIP_NATIVE_DEP_CHECK=1 there, or say so in review and I will move the call inside build-next-isolated.mjs behind the same flag.
  • extractServerExternalPackages parses next.config.mjs as text rather than importing it. Importing would pull in the next-intl and fumadocs-mdx plugins at module scope — the very dependency tree this gate runs before validating. The parser anchors on serverExternalPackages\s*:\s*\[ specifically because the file discusses the option in a comment ~130 lines above the real declaration.
  • The gate is scoped to optionalDependencies on purpose. A missing required dependency means npm install never ran, which is already loud everywhere else.
  • I have not yet added the changelog.d/ fragment, since the convention needs the PR number in the filename. I will push it as a follow-up commit here as soon as this PR is numbered.
  • Branched from release/v3.8.51. Happy to retarget if a different base is the current active release.

npm 11 refuses to run install scripts for optional dependencies unless approved, drops the package, and still exits 0. better-sqlite3 is externalised via serverExternalPackages, and the bundler must resolve an external before it can externalise it, so the build then fails minutes later with a misleading "Module not found".

This gate names the package and prints the fix in about a second.
Covers the three pure exports: array extraction (ordering, comment lines, single quotes, unterminated arrays, and the prose-mention regression), optional-only scoping with an injected resolver, and the report text. One case asserts against the real next.config.mjs so the gate cannot silently stop seeing better-sqlite3.
Source-level guard: isAndroid is declared exactly once and before the try, the catch still derives its reported timeout from it, and the manual-fix guidance stays reachable. Reads the source rather than importing it, because postinstall.mjs does its work at module scope and importing it would run the real postinstall as a test side effect.
The catch block reads isAndroid to report which timeout it used, but the constant was block-scoped to the try. On the rebuild-timeout path that threw ReferenceError: isAndroid is not defined, which escaped the catch, rejected the top-level await in this module, and failed the whole npm install — on the exact path that exists to print manual-fix hints for machines without a C++ toolchain.
Adds check:native-deps alongside the other check:* scripts, and a prebuild hook so npm runs it automatically ahead of build. It fails only where the build would have failed anyway, one second in instead of four minutes.

Note for reviewers: prebuild fires for `npm run build` but not for build:secure or build:backend, which call build-next-isolated.mjs directly. OMNIROUTE_SKIP_NATIVE_DEP_CHECK=1 opts out.
Copilot AI lite review requested due to automatic review settings August 28, 2026 05:24
One fragment per PR, filename prefixed with the PR number for deterministic aggregation, per changelog.d/README.md.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new gate’s success log message is currently misleading about what was actually validated, which can cause confusion during diagnosis.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR strengthens the build/install ergonomics around optional native dependencies (notably better-sqlite3) by adding a fast preflight check that fails early when npm silently drops an externalised optional dep, and by fixing a scope bug in postinstall that could turn a recoverable rebuild failure into a hard npm install failure.

Changes:

  • Add check:native-deps and wire it into prebuild to fail fast when an externalised optionalDependency is missing.
  • Fix scripts/build/postinstall.mjs so isAndroid is in-scope for the rebuild timeout reporting path.
  • Add unit tests covering the new preflight logic and the postinstall regression scenario.
File summaries
File Description
scripts/check/check-native-deps.mjs New preflight gate that cross-checks next.config.mjs serverExternalPackages vs package.json optionalDependencies and reports missing optional externals.
package.json Adds check:native-deps script and runs it via the prebuild hook.
scripts/build/postinstall.mjs Moves isAndroid declaration outside the try so the catch can report timeouts without throwing ReferenceError.
tests/unit/build/check-native-deps.test.ts New tests for parsing serverExternalPackages, missing-externals detection, and report formatting.
tests/unit/build/postinstall-rebuild-timeout-scope.test.ts New source-level regression test ensuring isAndroid remains declared once and outside the rebuild try.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +209 to +212
console.log(
`[check-native-deps] OK — all ${externals.length} externalised packages resolve ` +
`(${optionalDeps.length} optional deps checked).`
);
@diegosouzapw
diegosouzapw merged commit 3852e05 into cheaper-inference:release/v3.8.51 Aug 30, 2026
3 checks passed
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…lently dropped (cheaper-inference#11863)

Boarded with 8 other PRs in one combined worktree: typecheck:core, check:file-size, check:changelog-integrity, check:complexity, check:cognitive-complexity, check:cycles, check-native-deps all green; 75/75 focused tests pass. Fixes a genuinely confusing failure mode — matches the documented TROUBLESHOOTING.md symptom exactly. Thanks for the preflight check and clear diagnostics.
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.

3 participants