Skip to content

Comments

fix: add direct .node require fallback for Bun compiled binaries#356

Merged
kexi merged 2 commits intodevelopfrom
fix/native-module-direct-load-fallback
Feb 13, 2026
Merged

fix: add direct .node require fallback for Bun compiled binaries#356
kexi merged 2 commits intodevelopfrom
fix/native-module-direct-load-fallback

Conversation

@kexi
Copy link
Owner

@kexi kexi commented Feb 13, 2026

Summary

  • Add loadNativeForPlatform() fallback in native.ts that directly require()s .node files using string literals, bypassing NAPI-RS index.js's existsSync() check which can fail inside Bun's $bunfs virtual filesystem
  • Add VIBE_DEBUG diagnostic log for copy strategy selection in start.ts
  • Enhance beta release CI to run functional tests on native targets verifying the native module actually loads

Context

In Bun compiled binaries, the NAPI-RS generated index.js uses existsSync(join(__dirname, "*.node")) to check for .node files before loading them. Inside Bun's virtual filesystem ($bunfs), existsSync can return false, causing the native module load to silently fail and fall back to slow cp -cR instead of fast clonefile() syscall.

The fix adds a second loading path using string literal require() calls that Bun's bundler can statically analyze and embed during bun build --compile.

Test plan

  • pnpm run check:all passes (353 tests, lint, typecheck all green)
  • Local compiled binary loads native module successfully (available: true, supportsDirectory: true)
  • Verify via VIBE_DEBUG=1 vibe start <branch> that Copy strategy: clonefile is reported on macOS
  • CI functional test passes on native targets

🤖 Generated with Claude Code

kexi and others added 2 commits February 13, 2026 15:33
NAPI-RS generated index.js uses existsSync() to check for .node files,
which can fail inside Bun's virtual filesystem ($bunfs) in compiled
binaries. Add loadNativeForPlatform() as a fallback that uses string
literal require() paths, ensuring Bun's bundler statically discovers
and embeds the .node files during `bun build --compile`.

Also add VIBE_DEBUG diagnostic log for copy strategy selection.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Enhance the beta release workflow to run the compiled binary on native
targets (non-cross-compilation) with VIBE_DEBUG=1, verifying that the
native module actually loads at runtime rather than relying solely on
string search.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@kexi kexi merged commit 5c5f975 into develop Feb 13, 2026
20 checks passed
@kexi kexi deleted the fix/native-module-direct-load-fallback branch February 13, 2026 06:35
@claude
Copy link

claude bot commented Feb 13, 2026

PR Review: Fix native module loading in Bun compiled binaries

Summary

This PR addresses a critical issue where the native module fails to load inside Bun's compiled binary virtual filesystem ($bunfs) due to NAPI-RS's existsSync() check failing. The solution adds a fallback loading path using string literal require() calls that Bun's bundler can statically analyze.


Code Quality & Best Practices

Strengths:

  • Clean fallback pattern: Try NAPI-RS package first, then direct .node loading
  • Well-documented code with clear comments explaining the Bun bundler requirement
  • String literal require() calls enable static analysis (critical for bundling)
  • Proper error handling with VIBE_DEBUG diagnostics
  • Follows existing project patterns and conventions

Suggestions:

  1. Missing platform validation: The loadNativeForPlatform() function doesn't validate process.arch values. Consider handling unexpected architectures explicitly.

  2. Hardcoded relative paths: The paths like ../../../../native/vibe-native.*.node are fragile. If the file structure changes, all 4 require statements break. Consider documenting this constraint in a comment.


Potential Issues

Critical

None identified - the core logic is sound.

Minor

  1. Silent failures in CI cross-compilation: The functional test only runs on native targets. Cross-compiled binaries are verified via string search but not runtime behavior. This is acceptable given the constraint.

  2. Destructuring process properties: Line 41 extracts platform and arch, but these could theoretically be modified (unlikely but possible in exotic environments). Consider accessing process.platform/process.arch directly in conditions for defense-in-depth.


Performance Considerations

Excellent:

  • Lazy loading pattern preserved (only loads when needed)
  • Fallback adds negligible overhead (~4 additional checks after primary load fails)
  • Native CoW cloning performance unaffected once loaded
  • VIBE_DEBUG logging only when enabled (no production overhead)

Security Review

Pass - No security concerns identified:

  • No command injection risk: Uses require() with static string literals only
  • No path traversal: All paths are hardcoded relative paths within the package
  • No eval/dynamic code: No dynamic code execution
  • No new dependencies: Uses existing @kexi/vibe-native package
  • Proper error handling: Errors caught and logged, no information leakage

Aligns with project's security checklist (docs/SECURITY_CHECKLIST.md).


Test Coverage

Concerns:

  1. No unit tests for loadNativeForPlatform(): The new fallback function lacks dedicated tests. While the CI functional test validates end-to-end behavior, unit tests would improve coverage for each platform/arch combination, error handling, and debug logging.

  2. Existing test gap: No tests found for packages/core/src/runtime/node/native.ts. The copy.test.ts tests strategies but not the native loading logic directly.

Recommendation: Add tests in a follow-up PR (non-blocking).


Documentation Updates

Status: No updates needed

Checked:

  • README.md and README.ja.md: No changes needed (user-facing behavior unchanged)
  • Code comments: Excellent inline documentation explaining the Bun bundler constraint
  • CI workflow: Well-commented functional test logic

The PR body clearly explains the issue and solution.


License Compatibility

Status: Pass

  • No new dependencies introduced
  • Uses existing @kexi/vibe-native package (already in project)
  • No GPL or copyleft licenses introduced

CI Workflow Enhancement

Excellent addition:

  • String search verification (existing)
  • New: Functional test on native targets verifying module actually loads at runtime
  • Proper platform detection logic (darwin-arm64/x64, linux-arm64/x64)
  • Graceful skip for cross-compilation targets

Minor suggestion:
The platform detection bash script (lines 114-139) is complex. Consider extracting it to a separate script file for maintainability in a future refactoring.


Overall Assessment

Status: Approve with minor suggestions

Strengths:

  1. Solves a critical production issue (native module not loading in compiled binaries)
  2. Clean, well-documented implementation
  3. Enhanced CI verification catches regressions
  4. Zero security concerns
  5. No breaking changes or API modifications

Recommendations for follow-up:

  1. Add unit tests for loadNativeForPlatform()
  2. Consider refactoring CI platform detection to separate script
  3. Add explicit logging for unsupported platforms in debug mode

Merge recommendation: LGTM - Ready to merge after addressing test coverage in a follow-up PR (non-blocking).


Great work on identifying and fixing this Bun bundler issue! The solution is elegant and maintains backward compatibility while adding the necessary fallback path.

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.

1 participant