Repository navigation
Conversation
|
Updated 4:42 AM PT - Aug 28th, 2026
❌ @robobun, your commit 8264f6a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34475That installs a local version of the PR into your bun-34475 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesThe WASI shim now validates configuration, selects import namespaces, supports configured standard streams, exposes WASI runtime behavior
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/wasi.ts`:
- Around line 2009-2040: Move the kStarted state transition out of
finalizeBindings and into start and initialize after their _start/_initialize
export validation succeeds but before guest code is invoked. Keep
finalizeBindings responsible for memory and instance binding only, and ensure
validation failures leave the WASI instance reusable. Add a regression test
covering a failed export validation followed by a successful valid invocation.
- Around line 817-831: Cache the wasiConfig properties args, env, preopens, and
returnOnExit in local variables before any undefined checks or validation within
the option-processing flow. Use those locals for validateArray, validateObject,
validateBoolean, and args mapping so each getter is invoked only once, while
preserving the existing defaults and validation behavior.
In `@test/js/node/wasi/wasi.test.ts`:
- Around line 104-118: Expand the WASI option tests around the existing
stdin/stdout/stderr and returnOnExit cases: verify FD values 0 and 2 ** 31 - 1
are accepted while NaN, infinities, and 2 ** 31 are rejected; add a
returnOnExit: false proc_exit assertion covering the host-exit path; and add
stderr guest FD 2 coverage confirming writes use the configured host descriptor,
matching stdin/stdout tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40461bf3-090c-46aa-8662-70fffc1dd947
📒 Files selected for processing (5)
src/js/node/wasi.tssrc/js/wasi-runner.jssrc/jsc/bindings/ErrorCode.tstest/js/bun/wasm/wasi.test.jstest/js/node/wasi/wasi.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No further findings — all four earlier rounds of nits have been addressed. This is a moderate-scope Node compat change (new public methods, constructor now requires version, bun file.wasm now runs _initialize for reactors) with overlap against #34474/#34036, so leaving the merge decision to a maintainer.
Checked: constructor validation ordering vs Node v26 finalizeBindings; returnOnExit sentinel doesn't leak when _start throws a user error; stdio fd wiring reaches fd_write/fd_read via stats.real; wasi-runner.js reactor branch is covered by the new CLI test.
Extended reasoning...
Overview
Touches src/js/node/wasi.ts (constructor validation, start()/initialize()/getImportObject()/finalizeBindings(), sock_accept, stdio fd mapping, returnOnExit sentinel), src/js/wasi-runner.js (pass version/returnOnExit, branch on _start vs reactor), ErrorCode.ts (adds ERR_WASI_ALREADY_STARTED), plus a new 22-test file and updates to the existing WASI test to pass version.
Security risks
None introduced. The stdio fd options are validated with validateInt32(..., 0) before being stored; the WASI preopen sandbox logic is untouched. No new privilege surface.
Level of scrutiny
Medium. node:wasi is a JS-only compat shim (file header calls it a "quick hack"), not a hot path or memory-safety surface. But this is user-facing API: new WASI()/new WASI({}) now throw where they previously didn't (matching Node), start() now validates exports strictly, and bun reactor.wasm now calls _initialize where it used to silently no-op. Those are intentional and Node-aligned, but they change observable behavior for existing callers, which is worth a maintainer's sign-off.
Other factors
- Four prior review rounds from me (reactor handling in wasi-runner,
kEmptyObjectreuse,kStartedordering vs Node v26, bare.toThrow(),ArrayPrototypeMapidiom) — all addressed in de0bed0 → 1e088a5. - The
kStarted-ordering question was resolved against Node v26.3.0's actualfinalizeBindings(sets the flag after instance/exports/memory validation), which this PR matches. - Test coverage is thorough: exact error codes, fd boundary at
2**31-1,returnOnExit:falsevia spawned child, stdout/stderr/stdin fd routing including a subprocess assertion that guest fd 1 does not leak to host stdout, reactor via CLI. - Duplicate-PR bot flagged #34474 (fully subsumed) and #34036 (partial overlap on
getImportObject) — reconciling those is a maintainer call, not a code issue here.
…ortObject() The WASI class previously accepted any constructor input and silently diverged from Node's lifecycle contract: version/options were never validated, initialize() and getImportObject() didn't exist, start() returned undefined, ran on reactor modules, and could be called twice, and the documented stdin/stdout/stderr fd options were ignored so guest writes to fd 1 landed on the host's real stdout. Constructor now validates options/version/args/env/preopens/stdio/ returnOnExit the same way Node does; start() requires _start, rejects _initialize, runs once, and returns the exit code; initialize() and getImportObject() are implemented; the stdin/stdout/stderr options are wired into the fd map so guest stdio lands on the caller-supplied fds; returnOnExit (default true) captures proc_exit; sock_accept is present in wasiImport; ERR_WASI_ALREADY_STARTED is added.
bun file.wasm previously silently no-op'd on reactor-style modules (export _initialize, not _start) because the old start() tolerated a missing _start. With start() now matching Node's validation, branch on the module shape so reactors are driven via initialize() instead.
…n random_get test The dead-code sweep on main removed validateUndefined from internal/validators because nothing used it yet. WASI.start() and WASI.initialize() now use it. The new random_get test on main also constructs WASI without a version, which the constructor now rejects.
1e088a5 to
8264f6a
Compare
This branch has not been deployed
1 participant
What
node:wasi'sWASIclass looked like Node's but diverged at every lifecycle edge:new WASI({})/new WASI(42)/new WASI({version:"bogus"})TypeErrortypeof w.initialize/typeof w.getImportObject"function""undefined""sock_accept" in w.wasiImporttruefalsew.start(ok)0undefinedw.start(no_start)/w.start(reactor)TypeErrorundefinedw.start(i); w.start(i)ERR_WASI_ALREADY_STARTEDnew WASI({stdout: fd}), guest writes fd 1fdThe last one is the bad one: the
stdin/stdout/stderrfd options are the documented way to capture guest output. Ignoring them sends guest bytes into the embedder's own stdio.Fix
In
src/js/node/wasi.ts:options(object),options.version(required,"preview1"/"unstable"),args(array, elements coerced to strings),env/preopens(objects),stdin/stdout/stderr(non-negative int32),returnOnExit(boolean, defaulttrue), using the same error codes as Node.stdin/stdout/stderroptions are wired into the fd map so guest fds 0/1/2 back onto the supplied host fds.getImportObject()returns{wasi_snapshot_preview1: wasiImport}/{wasi_unstable: wasiImport}by version.finalizeBindings(instance, {memory})validatesinstance/instance.exports/memory, guards withERR_WASI_ALREADY_STARTED, and records the instance.start(instance)requires_startto be a function,_initializeto be undefined, catches theproc_exitsentinel whenreturnOnExitis on, and returns the exit code.initialize(instance)requires_startto be undefined, optionally calls_initialize.wasiImport.sock_acceptis present (returnsENOSYS, likesock_recv/sock_send).ERR_WASI_ALREADY_STARTEDis added to the error-code table.src/js/wasi-runner.js(thebun file.wasmentrypoint) now passesversion: "preview1"andreturnOnExit: falseto preserve its existing exit-code behavior.Overlap
#34474 implements the
returnOnExit/start()-returns-exit-code piece in isolation; this PR covers that as part of the wider lifecycle contract.Verification
test/js/node/wasi/wasi.test.ts(new, 28 tests) covers constructor validation (including fd boundaries and error codes),getImportObject(),start()return/validation/once semantics,returnOnExitin both modes,initialize()reactor handling, args coercion, and thestdin/stdout/stderrfd options (including a spawned child to assert guest fd 1 bytes do not reach host stdout). All fail on currentmain, all pass with this change.test/js/bun/wasm/wasi.test.js(updated to passversion, plus a newbun reactor.wasmCLI test) still passes.CI status: every lane is green except darwin x64, which fails only on
test/js/web/url/url.test.ts(ICU/IDNA). That test fails the same way onmainat 69c6138 and is unrelated to this change. It is reported for main-break triage.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/wasm/wasi.test.js
Fixes #12755
Fixes #28534
Fixes #40774