Skip to content

fix(rivetkit): omit native engine path for wasm runtime - #5626

Open
NathanFlurry wants to merge 1 commit into
mainfrom
fix-rivetkit-wasm-serve-config
Open

fix(rivetkit): omit native engine path for wasm runtime#5626
NathanFlurry wants to merge 1 commit into
mainfrom
fix-rivetkit-wasm-serve-config

Conversation

@NathanFlurry

Copy link
Copy Markdown
Member
  • Pass the npm Engine binary path only to the native runtime.
  • Preserve native local-Engine startup while allowing configured WASM registries to initialize.

@railway-app

railway-app Bot commented Aug 31, 2026

Copy link
Copy Markdown

🚅 Deployed to the actors-pr-5626 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-cloud 😴 Sleeping (View Logs) Web Aug 31, 2026 at 5:19 am
frontend-inspector 😴 Sleeping (View Logs) Web Aug 31, 2026 at 5:19 am
kitchen-sink 😴 Sleeping (View Logs) Web Aug 31, 2026 at 5:17 am
ladle ✅ Success (View Logs) Web Aug 31, 2026 at 5:10 am
mcp-hub ✅ Success (View Logs) Web Aug 31, 2026 at 5:08 am
website ❌ Build Failed (View Logs) Web Aug 31, 2026 at 5:07 am

@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Code Review

Reviewed this PR (fix(rivetkit): omit native engine path for wasm runtime) against the repo conventions in CLAUDE.md.

Summary: Small, well-scoped fix that gates native engine-binary-path resolution behind runtimeKind === "napi" in rivetkit-typescript/packages/rivetkit/src/registry/native.ts, so WASM registries no longer get (and do not need) a native binary path.

Findings: None survived review.

Verification performed:

  • CoreRuntime["kind"] is an exhaustive "napi" | "wasm" union, so gating on === "napi" correctly covers the wasm branch without needing a third case.
  • The single call site (buildRegistryWithRuntime) and both test call sites were updated to the new two-argument signature; no stale single-argument callers remain elsewhere in the repo.
  • RuntimeServeConfig.engineBinaryPath is optional, so leaving it unset for the wasm path is type-safe, and the new tests toBeUndefined() assertion is correct.
  • The Rust side (engine-process/src/lib.rs) independently re-reads RIVET_ENGINE_BINARY_PATH from the environment as a fallback, so omitting the JS-resolved path for wasm does not regress that override path.
  • This matches the documented convention to select runtime behavior from CoreRuntime.kind rather than instanceof adapter classes.

Test coverage looks appropriate for the change (registry-constructor test updated to assert the wasm path leaves engineBinaryPath undefined while native still resolves it). No security, performance, or style concerns found.

Generated with Claude Code

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