Merged
Conversation
This is like builtins.wasm, but the Wasm environment provides WASI modules. This may make it easier to build certain functions.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded optional WASI support to the Wasm host: instance preps and function registration accept a Changes
Sequence DiagramsequenceDiagram
participant EvalState as EvalState
participant Linker as Linker
participant WasmModule as WasmModule
participant WASI as WASI runtime
participant Host as env.return_to_nix
EvalState->>Linker: regFuns(linker, useWasi=true)
Linker->>WASI: define_wasi()
Linker->>Linker: register env.return_to_nix (trap)
EvalState->>WasmModule: instantiateWasm(wasmPath, useWasi=true)
WasmModule->>WASI: _start (argv includes input)
WASI->>WasmModule: execute module
WasmModule->>Host: call return_to_nix(result)
Host-->>EvalState: record resultId, emit logs (logPrefix)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libexpr/primops/wasm.cc`:
- Around line 571-614: The prim_wasi function currently only calls
wasiConfig.inherit_stderr(), which differs from the wasi-derivation-builder
usage (inherit_stdin(), inherit_stdout(), inherit_stderr()) and lacks
documentation explaining the stricter I/O restriction; either document the
intended design choice or change prim_wasi to match the other usage. Update the
prim_wasi implementation (function prim_wasi) to one of two fixes: (A) if
stricter isolation is intended, add a clear comment and a short runtime
note/error that explains the intentional omission of
inherit_stdin()/inherit_stdout() for purity/sandboxing, referencing wasiConfig
and the call site where inherit_stderr() is set, or (B) if full I/O should be
allowed, add wasiConfig.inherit_stdin() and wasiConfig.inherit_stdout()
alongside inherit_stderr() before set_wasi is called. Ensure the chosen approach
is documented in the function comment near prim_wasi and that the change is
applied around the wasiConfig setup and
unwrap(instance.wasmStore.context().set_wasi(...)) call so behavior is
consistent with the other builder.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/libexpr/primops/wasm.cc (2)
608-615: Inconsistent argument naming.The first argument is named
"wasi"but represents the Wasm module path, same asprimop_wasm's first argument named"wasm". Consider using"wasm"for consistency across both primitives.♻️ Suggested fix
static RegisterPrimOp primop_wasi( {.name = "wasi", - .args = {"wasi", "arg"}, + .args = {"wasm", "arg"}, .doc = R"( Call a WASI function with the specified argument. )",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops/wasm.cc` around lines 608 - 615, The argument name for primop_wasi is inconsistent: change its args array first element from "wasi" to "wasm" so it matches primop_wasm and clearly represents the Wasm module path; update the args in the static RegisterPrimOp primop_wasi declaration (the .args = {"wasi", "arg"}) to .args = {"wasm", "arg"} and keep the rest (including .fun = prim_wasi and .experimentalFeature) unchanged.
184-189: Redundantvalue_orinside guarded branch.Since this branch is only entered when
functionNamehas a value,value_or("<unknown>")is unnecessary. Use*functionNameinstead.♻️ Suggested fix
if (functionName) nix::warn( "'%s' function '%s': %s", pre->wasmPath, - functionName.value_or("<unknown>"), + *functionName, span2string(memory().subspan(ptr, len)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops/wasm.cc` around lines 184 - 189, The nix::warn call inside the if (functionName) guard redundantly uses functionName.value_or("<unknown>"); replace that with dereferencing the optional (use *functionName) in the nix::warn format argument so the logged function name uses the known value; update the nix::warn invocation that currently references pre->wasmPath, functionName.value_or("<unknown>"), and span2string(memory().subspan(ptr, len)) to use *functionName instead of value_or.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/libexpr/primops/wasm.cc`:
- Around line 588-591: The code in primops/wasm.cc sets up a WasiConfig that
only calls WasiConfig::inherit_stderr() (used in the prim_wasi path via
instance.wasmStore.context().set_wasi) while another module
(wasi-derivation-builder.cc) inherits stdin/stdout/stderr; document this
deliberate I/O restriction by adding a clear comment adjacent to the WasiConfig
creation in primops/wasm.cc (near the prim_wasi handling or the
instance.wasmStore.context().set_wasi call) that states the rationale for
inheriting only stderr, the security/isolation trade-offs, and a reference to
wasi-derivation-builder.cc behavior and any design doc or issue ticket for
future changes. Ensure the comment mentions the relevant symbols (prim_wasi,
WasiConfig::inherit_stderr, instance.wasmStore.context().set_wasi, and
wasi-derivation-builder.cc) so reviewers can trace the design choice.
---
Nitpick comments:
In `@src/libexpr/primops/wasm.cc`:
- Around line 608-615: The argument name for primop_wasi is inconsistent: change
its args array first element from "wasi" to "wasm" so it matches primop_wasm and
clearly represents the Wasm module path; update the args in the static
RegisterPrimOp primop_wasi declaration (the .args = {"wasi", "arg"}) to .args =
{"wasm", "arg"} and keep the rest (including .fun = prim_wasi and
.experimentalFeature) unchanged.
- Around line 184-189: The nix::warn call inside the if (functionName) guard
redundantly uses functionName.value_or("<unknown>"); replace that with
dereferencing the optional (use *functionName) in the nix::warn format argument
so the logged function name uses the known value; update the nix::warn
invocation that currently references pre->wasmPath,
functionName.value_or("<unknown>"), and span2string(memory().subspan(ptr, len))
to use *functionName instead of value_or.
cole-h
approved these changes
Feb 20, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libexpr/primops/wasm.cc`:
- Around line 609-648: The prim_wasi function configures a WasiConfig and calls
set_wasi on the store after instantiateWasm, but Wasmtime requires set_wasi to
be applied before any module instantiation that calls linker.define_wasi(); move
the WASI setup so it runs prior to instantiating the module. Specifically,
refactor prim_wasi to create WasiConfig, call
wasi_config_set_stdout_custom/wasi_config_set_stderr_custom and then call
unwrap(instance.wasmStore.context().set_wasi(...)) (or alternatively change
instantiateWasm to accept a WasiConfig and apply set_wasi inside instantiateWasm
before linker.define_wasi()); ensure the call sites using instantiateWasm (the
current prim_wasi invocation) pass the preconfigured WasiConfig or that
prim_wasi sets it before calling instantiateWasm so WASI is configured before
instantiation.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
doc/manual/source/protocols/wasm.md (1)
41-41: Consider clarifying edge cases for stdout/stderr capture.The documentation states that stdout/stderr are "emitted as Nix warnings (one warning per line)," but edge cases are unclear:
- How is output without a trailing newline handled?
- What happens to a partial line when the module exits?
- Is there line buffering that might delay output?
While these may be implementation details, brief clarification could help users understand logging behavior, especially for debugging. This is optional but would improve the documentation's completeness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/manual/source/protocols/wasm.md` at line 41, Update the wasm.md wording around the sentence "Standard output and standard error from the WASI module are captured and emitted as Nix warnings (one warning per line)" to explicitly cover edge cases: state how output without a trailing newline is handled (e.g., emitted as a final warning containing the partial line when the module exits), what happens to a partial line on process exit, and whether buffering/line-delimiting behavior can delay emission; reference the "WASI module" stdout/stderr capture behavior in that paragraph and add a short note about buffering semantics and that each captured line (or final partial line) is emitted as a Nix warning to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/manual/source/protocols/wasm.md`:
- Line 45: The sentence claiming "All host functions are imported from the `env`
module provided by `builtins.wasm`" is incomplete; update the line in
protocols/wasm.md referencing the `env` module so it mentions both providers
(for example: "All host functions are imported from the `env` module provided by
both `builtins.wasm` and `builtins.wasi`" or "provided by the Nix Wasm host")
and ensure the text references the `env` module, `builtins.wasm`, and
`builtins.wasi` so readers understand WASI modules get the same interface.
- Line 339: Clarify that return_to_nix transfers control back to the host
immediately (like an exit) and does not return to the module, so any in-module
cleanup, destructors, or finalizers will not run after it is invoked; update the
note around return_to_nix and _start to state explicitly that the module must
finish all cleanup before calling return_to_nix (or avoid calling it if using
normal return semantics), and give a short example guidance: call
destructors/cleanup in _start before invoking return_to_nix because
return_to_nix behaves as an immediate transfer of control to the host.
- Line 37: The docs are ambiguous about how ValueId is encoded in argv[1];
update the wasm protocol text (the sentence describing argv[1]) to explicitly
state the format (e.g., "argv[1] is the decimal string representation of the
input ValueId, e.g., '42'") so WASI modules know to parse argv[1] as a base-10
integer; reference argv[1] and ValueId in the updated sentence and add a brief
example to illustrate parsing.
- Around line 33-36: Add a clear note to the WASI docs stating that WASI modules
used with builtins.wasi do not export or call nix_wasm_init_v1(); instead they
follow the standard WASI convention where the host invokes _start() directly
with no parameters and the ValueId is provided as a decimal string in argv[1].
Mention the contrast with builtins.wasm (which uses nix_wasm_init_v1()) so
readers won't attempt to export nix_wasm_init_v1() in their WASI modules.
---
Nitpick comments:
In `@doc/manual/source/protocols/wasm.md`:
- Line 41: Update the wasm.md wording around the sentence "Standard output and
standard error from the WASI module are captured and emitted as Nix warnings
(one warning per line)" to explicitly cover edge cases: state how output without
a trailing newline is handled (e.g., emitted as a final warning containing the
partial line when the module exits), what happens to a partial line on process
exit, and whether buffering/line-delimiting behavior can delay emission;
reference the "WASI module" stdout/stderr capture behavior in that paragraph and
add a short note about buffering semantics and that each captured line (or final
partial line) is emitted as a Nix warning to aid debugging.
More determinism is good.
edolstra
force-pushed
the
eelcodolstra/nix-282-allow-wasm-plugins-to-use-wasi
branch
from
2bb726c to
428cbe3
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/libexpr/primops/wasm.cc`:
- Around line 609-647: The WASI configuration is being applied after
instantiateWasm(...) creates the module instance, which can cause traps when
WASI imports are required; modify prim_wasi so a preconfigured WasiConfig is set
on the store before instantiation: create a WasiConfig, call
wasi_config_set_stdout_custom / wasi_config_set_stderr_custom and then call
state.wasmStore.context().set_wasi(std::move(wasiConfig)) (or pass the
WasiConfig into the instantiateWasm path) before calling
instantiateWasm/InstancePre::instantiate; ensure instantiateWasm accepts or uses
the pre-set WasiConfig and only constructs the instance after set_wasi has been
applied.
This was referenced Feb 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This is like
builtins.wasm, but the Wasm environment provides WASI modules. This may make it easier to build certain functions.Context
Summary by CodeRabbit
New Features
Improvements
Documentation