Skip to content

Comments

chore: merge develop into main for v0.22.2#358

Merged
kexi merged 11 commits intomainfrom
develop
Feb 13, 2026
Merged

chore: merge develop into main for v0.22.2#358
kexi merged 11 commits intomainfrom
develop

Conversation

@kexi
Copy link
Owner

@kexi kexi commented Feb 13, 2026

Summary

Release v0.22.2 - merge develop into main.

Changes since last release (v0.22.1)

Test plan

  • All CI checks pass

🤖 Generated with Claude Code

kexi and others added 11 commits February 13, 2026 09:46
Regenerated index.js and index.d.ts with latest napi build output.
Adds moveToTrash/moveToTrashAsync exports and updates formatting.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add native module build step to release and beta-release workflows
so that bun build --compile includes the .node files in the binary.
This enables clonefile syscall for directory CoW cloning instead of
falling back to cp -cR on macOS/Linux.

- macOS: cross-compile for both x86_64 and aarch64
- Linux: build for current architecture only
- Windows: skip (vibe-native is macOS/Linux only)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Run prettier --write on index.d.ts and index.js to match project
code style.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
NAPI-RS auto-generation widened the return type to string.
Restore the specific union type for type safety.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add CI verification step to confirm native module embedding in release
binaries, clarify macOS cross-compilation comments in workflows, and
update documentation to reflect native clonefile() syscall usage.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…binary

fix: embed vibe-native in compiled release binaries
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>
fix: add direct .node require fallback for Bun compiled binaries
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Feb 13, 2026

PR Review: v0.22.2 - Native Module Embedding Fix

Summary

This PR merges develop into main for v0.22.2, fixing critical issues with native module embedding in compiled Bun binaries. The changes ensure that the @kexi/vibe-native module is properly embedded and loaded in release distributions.

Code Quality ✅

Strengths

  1. Well-documented fallback logic: The two-tier loading strategy in packages/core/src/runtime/node/native.ts is clearly explained with comments
  2. Proper error handling: Debug logging controlled via VIBE_DEBUG environment variable provides good observability
  3. Platform-specific handling: Correctly handles different architectures (x64, arm64) and platforms (darwin, linux)
  4. String literal requires: Correctly uses string literals in require() calls to enable Bun's static bundler analysis

Code Quality Observations

  1. Good separation of concerns: The loadNativeForPlatform() function is well-isolated and documented
  2. Lazy loading pattern: Properly implements singleton pattern with loadAttempted flag
  3. Cross-compilation aware: macOS CI builds both x64 and arm64 targets to ensure universal binary support

Potential Issues ⚠️

Minor Issues

  1. Missing Windows Architecture Handling (packages/core/src/runtime/node/native.ts:40-68)

    • The loadNativeForPlatform() function doesn't handle Windows, but this is by design since the early return on line 80-87 prevents Windows from reaching this code
    • Consider adding a comment in loadNativeForPlatform() explaining why Windows is not handled
  2. Duplicate Debug Logging (packages/core/src/runtime/node/native.ts:101-103)

    • Uses console.warn for successful load notification
    • Consider using console.error consistently or creating a dedicated debug logger
  3. Shell Script Complexity (.github/workflows/beta-release.yml:40-99)

    • The verification logic has complex nested conditions for target detection
    • Consider extracting this into a separate script file (e.g., scripts/verify-native-embedding.sh) for better maintainability and testability

Performance Considerations ✅

  1. Lazy loading: Native module is only loaded once and cached, avoiding repeated file system access
  2. CI optimization: On macOS, both architectures are built once in the same job, reducing CI time
  3. Build verification: String-based and functional verification steps catch embedding issues early

Security Concerns ✅

Native Module Security

  1. Path literals: Using string literals in require() prevents injection attacks
  2. Platform validation: Early Windows exit prevents unsupported platform access
  3. Error message sanitization: Error messages properly use String(error) to avoid object injection

CI Security

  1. No shell injection: Workflow properly uses ${{ matrix.artifact }} without additional shell expansion risk
  2. Verification steps: Binary verification with strings and functional tests catch tampering

Test Coverage ⚠️

Concerns

  1. No unit tests visible for the new loadNativeForPlatform() function

    • The fallback logic should have tests covering all platform/arch combinations
    • Mock tests for when .node files are missing vs present
  2. CI functional tests are limited

    • Only runs on native targets, cross-compilation targets skip functional verification
    • Consider adding integration tests that verify native module behavior

Recommendations

  • Add unit tests in packages/core/src/runtime/node/native.test.ts covering:
    • Successful load for each platform/arch combination (mocked)
    • Fallback behavior when direct load fails
    • Error message formatting

Documentation Updates ✅

README Updates (Synchronized)

Both README.md and README.ja.md were properly updated with:

  • Table update: "Clone (CoW)" strategy now mentions "Native clonefile()"
  • Detailed explanation of native module usage with fallback behavior
  • Excellent adherence to project's i18n synchronization rules

Changelog Updates (Synchronized)

Both packages/docs/src/content/docs/changelog.mdx and packages/docs/src/content/docs/ja/changelog.mdx properly document:

  • Version 0.22.2 release date
  • Bug fixes for native module embedding
  • Properly follows project's documentation i18n rules

Native Package Documentation

packages/native/README.md was updated with a new "Release Binary Embedding" section explaining the Bun compilation integration.

License Compatibility ✅

Analysis

Based on packages/native/Cargo.toml:

Direct Dependencies:

  • napi: MIT - ✅ Compatible
  • napi-derive: MIT - ✅ Compatible
  • thiserror: MIT OR Apache-2.0 - ✅ Compatible
  • tokio: MIT - ✅ Compatible
  • trash: MIT OR Apache-2.0 - ✅ Compatible
  • libc: MIT OR Apache-2.0 - ✅ Compatible
  • nix: MIT - ✅ Compatible

Verdict: No GPL-licensed dependencies detected. All dependencies use MIT or dual MIT/Apache-2.0 licenses, which are compatible with the project's Apache-2.0 license.

Additional Observations

Positive Aspects

  1. Incremental improvement: Builds on previous PR fix: embed vibe-native in compiled release binaries #355 and fix: add direct .node require fallback for Bun compiled binaries #356, showing good iterative development
  2. Build metadata tracking: The VIBE_DEBUG environment variable provides good debugging capability
  3. Cross-platform awareness: Properly handles architecture differences between CI runners and target platforms
  4. Production-ready: The verification steps in CI ensure that broken binaries won't be released

Suggestions for Future Work

  1. Add automated tests for the native module loading fallback logic
  2. Consider extracting CI verification logic into reusable scripts
  3. Add metrics/telemetry to track which loading path is used in production (package vs direct .node)

Verdict

✅ APPROVED - This is a solid bug fix release that addresses critical issues with native module embedding in compiled binaries. The implementation is well-thought-out with proper fallback mechanisms, the documentation is comprehensive and properly synchronized across languages, and there are no license compatibility concerns.

Minor Recommendations (Non-Blocking)

  1. Add unit tests for loadNativeForPlatform()
  2. Extract CI verification logic into a separate script
  3. Add inline comment in loadNativeForPlatform() about Windows handling

Great work on this release! 🎉

@kexi kexi merged commit 6ace21e into main Feb 13, 2026
49 checks passed
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