Conversation
Contributor
📝 WalkthroughWalkthroughThe PR restructures WASI plugin support by integrating two new plugins (nix-wasm-plugin-fib-wasi and nix-wasm-plugin-quickjs) into the main workspace, refactors the nix-wasm-rust API (renames Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 51-59: The flake currently hard-codes the Clang major version
("clang/19") in install paths; extract the major version from wasiCc.version
(e.g., parse wasiCc.version to get the major number into a variable like
clangMajor or clang_version_major) and use that variable instead of the literal
"19" wherever paths reference $out/lib/clang/19 (and the other occurrences
mentioned) so the links created (e.g., in the ln -s lines referencing
$out/lib/clang/<major>/include) and any other install targets automatically
follow the wasiCc version; ensure the parsing handles common version formats and
reuse the single variable in all place(s) that previously used "19".
In `@nix-wasm-plugin-quickjs/src/lib.rs`:
- Around line 10-55: The js_value_to_nix function is unbounded-recursive for
arrays/objects; add a recursion depth guard by changing js_value_to_nix to
accept a depth parameter (e.g., depth: usize) and a MAX_DEPTH constant, check at
the start and return a safe fallback (e.g., Value::make_null() or a controlled
error via fail) when depth == 0, and decrement depth on each recursive call into
arrays (Array::into_iter -> js_value_to_nix(entry, depth-1)) and objects
(object.props -> js_value_to_nix(value, depth-1)); update all call sites to pass
an initial MAX_DEPTH and ensure the conversion for lists (Value::make_list) and
attrsets (Value::make_attrset) uses the guarded recursive calls so deeply nested
or cyclic JS values stop at the configured depth.
- Line 2: The rquickjs types Value, Array, and Object now require the context
lifetime parameter; update the import and all usages to include the `'js`
lifetime (e.g., use rquickjs::{Array as Array<'js>, Object as Object<'js>, Value
as JsValue<'js>} or adjust type aliases) and then update any function
signatures, struct fields, and variable annotations that reference JsValue,
Array, or Object to JsValue<'js>, Array<'js>, and Object<'js> respectively
(ensure functions that introduce these types are generic over or bound to the
`'js` lifetime via fn foo<'js>(ctx: Ctx<'js>, ...) or appropriate context
types). Ensure no JS-tied value escapes its context by aligning lifetimes across
Context/Runtime usages (look for identifiers like JsValue, Array, Object,
Context in the diff and update them accordingly).
In `@nix-wasm-rust/src/lib.rs`:
- Around line 43-47: The parsed ValueId returned by wasi_arg() must be checked
for the sentinel zero before constructing a Value: after parsing into ValueId
(the variable value_id), add a guard that rejects 0 (e.g., panic or return an
error) with a clear message (similar style to the existing parse error) and only
call Value::from_raw(value_id) for non-zero ids; this prevents propagating an
invalid handle used elsewhere (see get_attr and Value::from_raw).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockwasi/Cargo.lockis excluded by!**/*.lockwasi/flake.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.vscode/tasks.jsonCargo.tomlflake.nixnix-wasm-plugin-fib-wasi/Cargo.tomlnix-wasm-plugin-fib-wasi/src/main.rsnix-wasm-plugin-fib-wasi/tests/fib.expnix-wasm-plugin-fib-wasi/tests/fib.nixnix-wasm-plugin-quickjs/Cargo.tomlnix-wasm-plugin-quickjs/src/lib.rsnix-wasm-plugin-quickjs/tests/eval.expnix-wasm-plugin-quickjs/tests/eval.nixnix-wasm-rust/src/lib.rswasi/Cargo.tomlwasi/flake.nixwasi/nix-wasi-plugin-fib/Cargo.toml
💤 Files with no reviewable changes (3)
- wasi/nix-wasi-plugin-fib/Cargo.toml
- wasi/flake.nix
- wasi/Cargo.toml
Comment on lines
+51
to
+59
| mkdir -p $out/bin $out/lib/clang/19 $out/share | ||
|
|
||
| ln -s ${wasiCc}/bin/wasm32-unknown-wasi-clang $out/bin/clang | ||
| ln -s ${wasiCc}/bin/wasm32-unknown-wasi-clang++ $out/bin/clang++ | ||
| ln -s ${wasiCc}/bin/wasm32-unknown-wasi-ar $out/bin/ar | ||
| ln -s ${wasiCc}/bin/wasm32-unknown-wasi-ld.lld $out/bin/ld.lld | ||
|
|
||
| ln -s ${wasiCc}/resource-root/include $out/lib/clang/19/include | ||
| ln -s ${wasiSysroot} $out/share/wasi-sysroot |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, find the flake.nix file
echo "=== Finding flake.nix ==="
fd -name "flake.nix" -type f
# Read the file to see its content
echo
echo "=== Reading flake.nix ==="
wc -l flake.nix && echo
# Show lines 40-70 to see context around lines 51-59
echo "=== Lines 40-70 (context for lines 51-59) ==="
sed -n '40,70p' flake.nix
# Show lines 105-125 to see context around lines 115-117
echo
echo "=== Lines 105-125 (context for lines 115-117) ==="
sed -n '105,125p' flake.nix
# Search for all hard-coded /lib/clang/19
echo
echo "=== All occurrences of /lib/clang/19 ==="
rg -n "clang/19" flake.nix || echo "No matches found"Repository: DeterminateSystems/nix-wasm-rust
Length of output: 349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, find the flake.nix file
echo "=== Finding flake.nix ==="
fd "flake.nix" -type f
# Read the file to see its content
echo
echo "=== File line count ==="
wc -l flake.nix
# Show lines 40-70 to see context around lines 51-59
echo
echo "=== Lines 40-70 (context for lines 51-59) ==="
sed -n '40,70p' flake.nix
# Show lines 105-125 to see context around lines 115-117
echo
echo "=== Lines 105-125 (context for lines 115-117) ==="
sed -n '105,125p' flake.nix
# Search for all hard-coded /lib/clang/19
echo
echo "=== All occurrences of clang/19 ==="
rg "clang/19" flake.nix || echo "No matches found"Repository: DeterminateSystems/nix-wasm-rust
Length of output: 289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if flake.nix exists in current directory
echo "=== Checking for flake.nix ==="
ls -la flake.nix
# Get line count
echo
echo "=== File line count ==="
wc -l flake.nix
# Show lines 40-70 to see context around lines 51-59
echo
echo "=== Lines 40-70 (context for lines 51-59) ==="
sed -n '40,70p' flake.nix
# Show lines 105-125 to see context around lines 115-117
echo
echo "=== Lines 105-125 (context for lines 115-117) ==="
sed -n '105,125p' flake.nix
# Search for all hard-coded /lib/clang/19
echo
echo "=== All occurrences of clang/19 ==="
rg "clang/19" flake.nix || echo "No matches found"Repository: DeterminateSystems/nix-wasm-rust
Length of output: 3305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the beginning of flake.nix to see imports
echo "=== Lines 1-40 (imports and setup) ==="
sed -n '1,40p' flake.nix
# Check if lib is imported and what's available
echo
echo "=== Search for 'lib' usage in file ==="
rg "lib\." flake.nix | head -20
# Search specifically for versions function
echo
echo "=== Search for 'versions' in file ==="
rg "versions" flake.nix || echo "No matches found"Repository: DeterminateSystems/nix-wasm-rust
Length of output: 1809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check full context around the wasiCc definition and the 'with pkgs;' scope
echo "=== Lines 24-45 (context showing 'with pkgs;' and wasiCc definition) ==="
sed -n '24,45p' flake.nix
# Check what happens with lib in context of nix-wasi-plugins derivation
echo
echo "=== Lines 128-132 (nix-wasm-plugins with pkgs; scope) ==="
sed -n '128,132p' flake.nix
# Search for any version extraction patterns in the file
echo
echo "=== Search for .version usage ==="
rg "\.version" flake.nix
# Check if the proposed lib.versions.major could work by looking at pkgs imports
echo
echo "=== Full grep for 'with pkgs' ==="
rg -A2 "with pkgs" flake.nix | head -30Repository: DeterminateSystems/nix-wasm-rust
Length of output: 1972
🏁 Script executed:
#!/bin/bash
set -eulo pipefail
# Check if wasiCc is actually accessible with .version attribute
# by looking at how stdenv.cc typically has version
echo "=== Check nixpkgs documentation or common patterns ==="
echo "In nixpkgs, stdenv.cc typically has .version attribute."
echo "lib.versions.major is a standard function in nixpkgs nixpkgs.lib"
echo
# Let's verify the exact structure by checking if there are similar patterns
echo "=== Search for any version string patterns in flake ==="
rg "version\s*=" flake.nix
# Check the full scope of nix-wasi-plugins to understand where lib would come from
echo
echo "=== Lines 24-26 (context of with pkgs scope) ==="
sed -n '24,26p' flake.nix
# Let's see the exact line numbers of the problem lines
echo
echo "=== Show exact problem lines with line numbers ==="
sed -n '51p; 57p; 115p; 116p' flake.nix | cat -nRepository: DeterminateSystems/nix-wasm-rust
Length of output: 1165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's verify the proposed solution more carefully
# Check what's available in scope at line 42 where clangMajor would be added
echo "=== Lines 24-65 (full nix-wasi-plugins derivation context) ==="
sed -n '24,65p' flake.nix
# Also check if inputs.nixpkgs.lib is available at the top level
echo
echo "=== Lines 9-15 (outputs function and available inputs) ==="
sed -n '9,15p' flake.nixRepository: DeterminateSystems/nix-wasm-rust
Length of output: 2549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify that lib.versions.major is a standard nix function
# by checking if it's a widely recognized pattern
echo "=== Verify lib.versions.major exists in nixpkgs ==="
echo "lib.versions.major is a standard function in nixpkgs.lib"
echo "It takes a version string and returns the major version number"
echo
# Let's also check what the actual clang version is in the nixpkgs being used
echo "=== Check the nix input for nixpkgs ==="
sed -n '1,10p' flake.nix | grep -A5 "inputs"
# Verify that within 'with pkgs;' scope, lib is indeed available
echo
echo "=== Confirm lib is available in 'with pkgs;' scope ==="
echo "Within 'with pkgs;' scope, all attributes of pkgs become available"
echo "lib is a standard attribute in pkgs, so lib.versions.major should work"
echo
# Count total occurrences to ensure we haven't missed any
echo "=== Final verification: count all clang/19 occurrences ==="
rg -c "clang/19" flake.nix || echo "0"Repository: DeterminateSystems/nix-wasm-rust
Length of output: 777
Avoid hard-coding the Clang major version in WASI SDK paths.
Lines 51 and 57 hard-code clang/19, as do lines 115–116, creating fragility when nixpkgs updates Clang. Derive the major version from wasiCc.version once and reuse it.
♻️ Proposed refactor
wasiCc = pkgs.pkgsCross.wasi32.stdenv.cc;
wasiLibc = pkgs.pkgsCross.wasi32.wasilibc;
wasiLibcDev = wasiLibc.dev;
+ clangMajor = lib.versions.major wasiCc.version;
wasiSysroot = runCommand "wasi-sysroot" { } ''
mkdir -p $out/include $out/lib/wasm32-wasip1
cp -R ${wasiLibcDev}/include/* $out/include/
cp -R ${wasiLibc}/lib/* $out/lib/
cp -R ${wasiLibc}/lib/* $out/lib/wasm32-wasip1/
'';
wasiSdk = runCommand "wasi-sdk-compat" { } ''
- mkdir -p $out/bin $out/lib/clang/19 $out/share
+ mkdir -p $out/bin $out/lib/clang/${clangMajor} $out/share
ln -s ${wasiCc}/bin/wasm32-unknown-wasi-clang $out/bin/clang
ln -s ${wasiCc}/bin/wasm32-unknown-wasi-clang++ $out/bin/clang++
ln -s ${wasiCc}/bin/wasm32-unknown-wasi-ar $out/bin/ar
ln -s ${wasiCc}/bin/wasm32-unknown-wasi-ld.lld $out/bin/ld.lld
- ln -s ${wasiCc}/resource-root/include $out/lib/clang/19/include
+ ln -s ${wasiCc}/resource-root/include $out/lib/clang/${clangMajor}/include
ln -s ${wasiSysroot} $out/share/wasi-sysroot
'';
@@
- CFLAGS_wasm32_wasip1 = "--sysroot=${wasiSdk}/share/wasi-sysroot -isystem ${wasiSdk}/lib/clang/19/include";
- BINDGEN_EXTRA_CLANG_ARGS_wasm32_wasip1 = "-fvisibility=default --sysroot=${wasiSdk}/share/wasi-sysroot -isystem ${wasiSdk}/lib/clang/19/include -resource-dir ${wasiSdk}/lib/clang/19";
+ CFLAGS_wasm32_wasip1 = "--sysroot=${wasiSdk}/share/wasi-sysroot -isystem ${wasiSdk}/lib/clang/${clangMajor}/include";
+ BINDGEN_EXTRA_CLANG_ARGS_wasm32_wasip1 = "-fvisibility=default --sysroot=${wasiSdk}/share/wasi-sysroot -isystem ${wasiSdk}/lib/clang/${clangMajor}/include -resource-dir ${wasiSdk}/lib/clang/${clangMajor}";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 51 - 59, The flake currently hard-codes the Clang
major version ("clang/19") in install paths; extract the major version from
wasiCc.version (e.g., parse wasiCc.version to get the major number into a
variable like clangMajor or clang_version_major) and use that variable instead
of the literal "19" wherever paths reference $out/lib/clang/19 (and the other
occurrences mentioned) so the links created (e.g., in the ln -s lines
referencing $out/lib/clang/<major>/include) and any other install targets
automatically follow the wasiCc version; ensure the parsing handles common
version formats and reuse the single variable in all place(s) that previously
used "19".
| @@ -0,0 +1,84 @@ | |||
| use nix_wasm_rust::{nix_wasm_init_v1, warn, wasi_arg, Value}; | |||
| use rquickjs::{Array, Context, Object, Runtime, Value as JsValue}; | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Cargo.toml" -path "*nix-wasm-plugin-quickjs*" | head -5Repository: DeterminateSystems/nix-wasm-rust
Length of output: 112
🏁 Script executed:
cat nix-wasm-plugin-quickjs/Cargo.toml | grep -A 5 -B 5 rquickjsRepository: DeterminateSystems/nix-wasm-rust
Length of output: 225
🏁 Script executed:
head -80 nix-wasm-plugin-quickjs/src/lib.rsRepository: DeterminateSystems/nix-wasm-rust
Length of output: 2782
🌐 Web query:
rquickjs 0.11.0 type signatures Value Array Object lifetime parameters
💡 Result:
In rquickjs 0.11.0, the core JS handle types all carry the context-tied lifetime parameter 'js:
Value<'js>:pub struct Value<'js> { /* private fields */ }(any JS value) and constructors take aCtx<'js>and returnValue<'js>(e.g.new_undefined(ctx: Ctx<'js>) -> Value<'js>). [1]Object<'js>:pub struct Object<'js>(/* private fields */);[2]Array<'js>:pub struct Array<'js>(/* private fields */);and e.g.Array::new(ctx: Ctx<'js>) -> Result<Array<'js>, Error>. [3]
The lifetime plumbing is formalized by JsLifetime:
pub unsafe trait JsLifetime<'js> {
type Changed<'to>: 'to;
}This trait is described as the “'js lifetime trick” used to keep JS values from outliving the JS context they come from. [4]
For these types specifically, JsLifetime is implemented so you can “retag” the lifetime at the type level:
Array<'js>→Changed<'to> = Array<'to>[5]Object<'js>→Changed<'to> = Object<'to>[5]Value<'js>→Changed<'to> = Value<'to>[5]
Meaning of 'js in practice: you generally can’t store/return a Value<'js> / Object<'js> / Array<'js> in a way that outlives the Ctx<'js> it’s associated with (the compiler enforces this via 'js). [1][4]
Sources: [1] Value docs, [2] Object docs, [3] Array docs, [4] JsLifetime docs, [5] JsLifetime implementors for Array/Object/Value.
rquickjs type annotations require explicit 'js lifetime parameters in version 0.11.0.
In rquickjs 0.11.0, Value<'js>, Array<'js>, and Object<'js> all require the context-tied lifetime parameter. The current code uses these types without lifetimes (JsValue, Array, Object), which will fail to compile. The lifetime parameter enforces that values cannot outlive their associated context.
Required fixes
-fn js_value_to_nix(value: JsValue) -> Value {
+fn js_value_to_nix<'js>(value: JsValue<'js>) -> Value {
if value.is_null() || value.is_undefined() {
return Value::make_null();
}
...
if value.is_array() {
- let array: Array = value
+ let array: Array<'js> = value
.clone()
.into_array()
.unwrap_or_else(|| fail("array conversion", "value is not an array"));
...
if value.is_object() {
- let object: Object = value
+ let object: Object<'js> = value
.into_object()
.unwrap_or_else(|| fail("object conversion", "value is not an object"));
let mut entries: Vec<(StdString, Value)> = Vec::new();
- for entry in object.props::<StdString, JsValue>() {
+ for entry in object.props::<StdString, JsValue<'js>>() {
...
let value: JsValue = ctx.eval(code)
+ let value: JsValue<'_> = ctx.eval(code)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nix-wasm-plugin-quickjs/src/lib.rs` at line 2, The rquickjs types Value,
Array, and Object now require the context lifetime parameter; update the import
and all usages to include the `'js` lifetime (e.g., use rquickjs::{Array as
Array<'js>, Object as Object<'js>, Value as JsValue<'js>} or adjust type
aliases) and then update any function signatures, struct fields, and variable
annotations that reference JsValue, Array, or Object to JsValue<'js>,
Array<'js>, and Object<'js> respectively (ensure functions that introduce these
types are generic over or bound to the `'js` lifetime via fn foo<'js>(ctx:
Ctx<'js>, ...) or appropriate context types). Ensure no JS-tied value escapes
its context by aligning lifetimes across Context/Runtime usages (look for
identifiers like JsValue, Array, Object, Context in the diff and update them
accordingly).
Comment on lines
+10
to
+55
| fn js_value_to_nix(value: JsValue) -> Value { | ||
| if value.is_null() || value.is_undefined() { | ||
| return Value::make_null(); | ||
| } | ||
| if let Some(b) = value.as_bool() { | ||
| return Value::make_bool(b); | ||
| } | ||
| if let Some(i) = value.as_int() { | ||
| return Value::make_int(i as i64); | ||
| } | ||
| if let Some(f) = value.as_float() { | ||
| return Value::make_float(f); | ||
| } | ||
| if let Some(js_str) = value.as_string() { | ||
| let s = js_str | ||
| .to_string() | ||
| .unwrap_or_else(|err| fail("string conversion", err)); | ||
| return Value::make_string(&s); | ||
| } | ||
| if value.is_array() { | ||
| let array: Array = value | ||
| .clone() | ||
| .into_array() | ||
| .unwrap_or_else(|| fail("array conversion", "value is not an array")); | ||
| let mut items = Vec::new(); | ||
| for entry in array.into_iter() { | ||
| let entry = entry.unwrap_or_else(|err| fail("array iteration", err)); | ||
| items.push(js_value_to_nix(entry)); | ||
| } | ||
| return Value::make_list(&items); | ||
| } | ||
| if value.is_object() { | ||
| let object: Object = value | ||
| .into_object() | ||
| .unwrap_or_else(|| fail("object conversion", "value is not an object")); | ||
| let mut entries: Vec<(StdString, Value)> = Vec::new(); | ||
| for entry in object.props::<StdString, JsValue>() { | ||
| let (key, value) = entry.unwrap_or_else(|err| fail("object iteration", err)); | ||
| entries.push((key, js_value_to_nix(value))); | ||
| } | ||
| let attrs: Vec<(&str, Value)> = entries | ||
| .iter() | ||
| .map(|(key, value)| (key.as_str(), *value)) | ||
| .collect(); | ||
| return Value::make_attrset(&attrs); | ||
| } |
Contributor
There was a problem hiding this comment.
Add a recursion guard for JS→Nix conversion.
js_value_to_nix is unbounded recursive for arrays/objects. Deeply nested or cyclic JS values can cause stack overflow and hard-fail the plugin.
Suggested patch (depth guard)
+const MAX_CONVERSION_DEPTH: usize = 256;
+
fn js_value_to_nix(value: JsValue) -> Value {
+ js_value_to_nix_with_depth(value, 0)
+}
+
+fn js_value_to_nix_with_depth(value: JsValue, depth: usize) -> Value {
+ if depth > MAX_CONVERSION_DEPTH {
+ fail("value conversion", "maximum conversion depth exceeded");
+ }
+
if value.is_null() || value.is_undefined() {
return Value::make_null();
}
@@
for entry in array.into_iter() {
let entry = entry.unwrap_or_else(|err| fail("array iteration", err));
- items.push(js_value_to_nix(entry));
+ items.push(js_value_to_nix_with_depth(entry, depth + 1));
}
return Value::make_list(&items);
}
@@
for entry in object.props::<StdString, JsValue>() {
let (key, value) = entry.unwrap_or_else(|err| fail("object iteration", err));
- entries.push((key, js_value_to_nix(value)));
+ entries.push((key, js_value_to_nix_with_depth(value, depth + 1)));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nix-wasm-plugin-quickjs/src/lib.rs` around lines 10 - 55, The js_value_to_nix
function is unbounded-recursive for arrays/objects; add a recursion depth guard
by changing js_value_to_nix to accept a depth parameter (e.g., depth: usize) and
a MAX_DEPTH constant, check at the start and return a safe fallback (e.g.,
Value::make_null() or a controlled error via fail) when depth == 0, and
decrement depth on each recursive call into arrays (Array::into_iter ->
js_value_to_nix(entry, depth-1)) and objects (object.props ->
js_value_to_nix(value, depth-1)); update all call sites to pass an initial
MAX_DEPTH and ensure the conversion for lists (Value::make_list) and attrsets
(Value::make_attrset) uses the guarded recursive calls so deeply nested or
cyclic JS values stop at the configured depth.
Comment on lines
+43
to
+47
| let value_id = arg | ||
| .parse::<ValueId>() | ||
| .unwrap_or_else(|err| panic(&format!("invalid WASI argument '{arg}': {err}"))); | ||
| Value::from_raw(value_id) | ||
| } |
Contributor
There was a problem hiding this comment.
Validate ValueId is non-zero before constructing Value.
wasi_arg() currently accepts 0, but 0 is treated as a sentinel invalid/missing value elsewhere (e.g., get_attr). Rejecting 0 here avoids propagating invalid handles.
Suggested patch
pub fn wasi_arg() -> Value {
let arg = std::env::args()
.nth(1)
.unwrap_or_else(|| panic("missing WASI argument"));
let value_id = arg
.parse::<ValueId>()
.unwrap_or_else(|err| panic(&format!("invalid WASI argument '{arg}': {err}")));
+ if value_id == 0 {
+ panic("invalid WASI argument: ValueId must be non-zero");
+ }
Value::from_raw(value_id)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let value_id = arg | |
| .parse::<ValueId>() | |
| .unwrap_or_else(|err| panic(&format!("invalid WASI argument '{arg}': {err}"))); | |
| Value::from_raw(value_id) | |
| } | |
| let value_id = arg | |
| .parse::<ValueId>() | |
| .unwrap_or_else(|err| panic(&format!("invalid WASI argument '{arg}': {err}"))); | |
| if value_id == 0 { | |
| panic("invalid WASI argument: ValueId must be non-zero"); | |
| } | |
| Value::from_raw(value_id) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nix-wasm-rust/src/lib.rs` around lines 43 - 47, The parsed ValueId returned
by wasi_arg() must be checked for the sentinel zero before constructing a Value:
after parsing into ValueId (the variable value_id), add a guard that rejects 0
(e.g., panic or return an error) with a clear message (similar style to the
existing parse error) and only call Value::from_raw(value_id) for non-zero ids;
this prevents propagating an invalid handle used elsewhere (see get_attr and
Value::from_raw).
detsys-pr-bot
approved these changes
Feb 28, 2026
grahamc
approved these changes
Feb 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nix-wasm-plugin-fib-wasi/src/main.rs (1)
25-30:⚠️ Potential issue | 🔴 CriticalGuard
argv[1]before indexing.Line 27 accesses
args[1]without a bounds check; invoking the binary without that argument will panic.Proposed fix
use nix_wasm_plugin_fib::fib; use nix_wasm_rust::Value; use std::env; +use std::process; fn main() { @@ - let args: Vec<String> = env::args().collect(); - let arg = Value::from_raw( - args[1] - .parse::<u32>() - .expect("argv[1] should be a valid ValueId"), - ); + let arg_raw = match env::args().nth(1) { + Some(v) => v, + None => { + eprintln!("missing argv[1]: expected ValueId"); + process::exit(2); + } + }; + let arg = Value::from_raw( + arg_raw + .parse::<u32>() + .expect("argv[1] should be a valid ValueId"), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nix-wasm-plugin-fib-wasi/src/main.rs` around lines 25 - 30, The code indexes args[1] without checking args.len(), which will panic if the program is invoked without that argument; modify the main function to guard access by checking args.len() or using args.get(1) and handling the None case (e.g., print a usage error and exit or return a Result). Ensure the guarded value is then parsed as before and passed into Value::from_raw, keeping the existing parse::<u32>().expect only for parse failures, and refer to the symbols env::args(), args, args.get(1), and Value::from_raw when making the change.
🧹 Nitpick comments (2)
flake.nix (2)
96-101: Consider documenting WASM feature requirements.The
wasm-optflags (--enable-bulk-memory,--enable-nontrapping-float-to-int,--enable-simd) enable WASM features that require runtime support. If downstream consumers need to know the minimum WASM runtime version, consider documenting these requirements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 96 - 101, The installPhase currently invokes wasm-opt with feature flags (--enable-bulk-memory, --enable-nontrapping-float-to-int, --enable-simd) that require runtime support; add documentation describing these required WASM features and minimum runtime versions (or engine flags) so downstream consumers know compatibility constraints. Update the flake.nix by adding a brief comment above the installPhase referencing the exact flags and recommended minimum engine versions (e.g., wasmtime/node/chrome versions) and also add a short entry to the repository README (or a new WASM.md) listing the same flags, the reason they're used, and links to relevant runtime/feature docs so consumers can verify their environment supports installPhase's outputs.
87-88: Address the FIXME: Filter workspace to include only WASI plugins.The comment indicates that non-WASI plugins should be filtered from the source. Currently, the entire workspace is passed as
src, which may include unnecessary files and could affect build reproducibility.Would you like me to help create a source filter that includes only the WASI plugin crates (
nix-wasm-plugin-quickjs,nix-wasm-plugin-fib-wasi) and their dependencies? Alternatively, I can open an issue to track this cleanup task.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 87 - 88, Replace the blanket workspace source assignment "src = self" with a filtered source that includes only the WASI plugin crates and their dependencies: use Nix's filterSource (or lib.filterSource) to match crate directories named "nix-wasm-plugin-quickjs" and "nix-wasm-plugin-fib-wasi" and exclude other workspace members; ensure the filter preserves Cargo.toml/Cargo.lock and any shared crates those plugins depend on so builds remain reproducible. Locate the line "src = self" in flake.nix and replace it by constructing a filtered source using filterSource with a predicate that returns true for the two plugin directories and for any path under their dependency subtrees.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 25-26: The flake references a non-existent attribute
rustPackages_1_89 causing build failures; replace it by either updating the
pinned nixpkgs in flake.lock to a revision that provides rustPackages_1_89, or
change the flake to use the standard pkg attributes (e.g. pkgs.rustc and
pkgs.cargo) and derive rustPlatform from pkgs (replace rustPackages =
pkgs.rustPackages_1_89; rustPlatform = rustPackages.rustPlatform; with an
approach that uses the updated nixpkgs or pkgs.rustc/pkgs.cargo to construct the
rustPlatform).
---
Outside diff comments:
In `@nix-wasm-plugin-fib-wasi/src/main.rs`:
- Around line 25-30: The code indexes args[1] without checking args.len(), which
will panic if the program is invoked without that argument; modify the main
function to guard access by checking args.len() or using args.get(1) and
handling the None case (e.g., print a usage error and exit or return a Result).
Ensure the guarded value is then parsed as before and passed into
Value::from_raw, keeping the existing parse::<u32>().expect only for parse
failures, and refer to the symbols env::args(), args, args.get(1), and
Value::from_raw when making the change.
---
Nitpick comments:
In `@flake.nix`:
- Around line 96-101: The installPhase currently invokes wasm-opt with feature
flags (--enable-bulk-memory, --enable-nontrapping-float-to-int, --enable-simd)
that require runtime support; add documentation describing these required WASM
features and minimum runtime versions (or engine flags) so downstream consumers
know compatibility constraints. Update the flake.nix by adding a brief comment
above the installPhase referencing the exact flags and recommended minimum
engine versions (e.g., wasmtime/node/chrome versions) and also add a short entry
to the repository README (or a new WASM.md) listing the same flags, the reason
they're used, and links to relevant runtime/feature docs so consumers can verify
their environment supports installPhase's outputs.
- Around line 87-88: Replace the blanket workspace source assignment "src =
self" with a filtered source that includes only the WASI plugin crates and their
dependencies: use Nix's filterSource (or lib.filterSource) to match crate
directories named "nix-wasm-plugin-quickjs" and "nix-wasm-plugin-fib-wasi" and
exclude other workspace members; ensure the filter preserves
Cargo.toml/Cargo.lock and any shared crates those plugins depend on so builds
remain reproducible. Locate the line "src = self" in flake.nix and replace it by
constructing a filtered source using filterSource with a predicate that returns
true for the two plugin directories and for any path under their dependency
subtrees.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockwasi/Cargo.lockis excluded by!**/*.lockwasi/flake.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/workflows/ci.yml.vscode/tasks.jsonCargo.tomlflake.nixnix-wasm-plugin-fib-wasi/Cargo.tomlnix-wasm-plugin-fib-wasi/src/main.rsnix-wasm-plugin-fib-wasi/tests/fib.expnix-wasm-plugin-fib-wasi/tests/fib.nixwasi/Cargo.tomlwasi/flake.nixwasi/nix-wasi-plugin-fib/Cargo.toml
💤 Files with no reviewable changes (4)
- wasi/Cargo.toml
- wasi/nix-wasi-plugin-fib/Cargo.toml
- .github/workflows/ci.yml
- wasi/flake.nix
✅ Files skipped from review due to trivial changes (1)
- nix-wasm-plugin-fib-wasi/tests/fib.exp
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- nix-wasm-plugin-fib-wasi/tests/fib.nix
- .vscode/tasks.json
Comment on lines
+25
to
+26
| rustPackages = pkgs.rustPackages_1_89; | ||
| rustPlatform = rustPackages.rustPlatform; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify rustPackages_1_89 exists in nixpkgs
# First, check available rust versions in nixpkgs
echo "=== Checking rustPackages attributes in nixpkgs ==="
nix eval --impure --expr 'let pkgs = import <nixpkgs> {}; in builtins.filter (x: builtins.match "rustPackages_[0-9]+_[0-9]+" x != null) (builtins.attrNames pkgs)' 2>&1 | head -50
echo ""
echo "=== Attempting direct access to rustPackages_1_89 ==="
nix eval --impure --expr '(import <nixpkgs> {}).rustPackages_1_89 or "NOT_FOUND"' 2>&1Repository: DeterminateSystems/nix-wasm-rust
Length of output: 269
🏁 Script executed:
#!/bin/bash
# Read flake.nix to understand the context around lines 25-26
echo "=== Examining flake.nix around the rustPackages_1_89 reference ==="
head -35 flake.nix | tail -20
echo ""
echo "=== Checking flake.lock for nixpkgs input ==="
if [ -f flake.lock ]; then
cat flake.lock | grep -A 10 '"nixpkgs"'
fi
echo ""
echo "=== Searching for rustPackages references in the repo ==="
rg "rustPackages" --type nix -A 2 -B 2Repository: DeterminateSystems/nix-wasm-rust
Length of output: 4875
🌐 Web query:
Rust 1.89 release date nixpkgs rustPackages_1_89
💡 Result:
-
Rust 1.89.0 release date: August 7, 2025. [1] (also confirmed by the 1.89.0 pre-release announcement stating the release was scheduled for Aug 7) [2]
-
nixpkgs
rustPackages_1_89: nixpkgs does not generally provide arustPackages_1_XXscope for every Rust version; older pinned scopes exist for some versions (example list includesrustPackages,rustPackages_1_45,rustPackages_1_48, etc.), butrustPackages_1_89is not a standard top-level attribute you can rely on. [3] -
How to get Rust 1.89 from nixpkgs anyway (pin a revision that packaged it):
- Rust 1.89.0 is available in nixpkgs at revision
01b6809f7f9d1183a2b3e081f0a1e6f8f415cb09(as#rustc). [4] - Practically: pin nixpkgs to that commit and use
pkgs.rustc/pkgs.cargo(or that revision’spkgs.rustPlatform) rather than expectingrustPackages_1_89. [4]
- Rust 1.89.0 is available in nixpkgs at revision
Sources: [1] [2] [3] [4]
rustPackages_1_89 does not exist in the pinned nixpkgs version and will cause build failures.
The flake.lock pins nixpkgs to revision daf6dc47... (September 2024), which predates Rust 1.89's release (August 2025). Additionally, nixpkgs does not maintain rustPackages_1_XX attributes for every Rust version. Either update the nixpkgs pin to a recent revision that includes Rust 1.89, or use pkgs.rustc and pkgs.cargo directly instead of the non-existent rustPackages_1_89 scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 25 - 26, The flake references a non-existent
attribute rustPackages_1_89 causing build failures; replace it by either
updating the pinned nixpkgs in flake.lock to a revision that provides
rustPackages_1_89, or change the flake to use the standard pkg attributes (e.g.
pkgs.rustc and pkgs.cargo) and derive rustPlatform from pkgs (replace
rustPackages = pkgs.rustPackages_1_89; rustPlatform = rustPackages.rustPlatform;
with an approach that uses the updated nixpkgs or pkgs.rustc/pkgs.cargo to
construct the rustPlatform).
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
4 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.
Includes #4. Merges the wasi subflake into the quickjs derivation to simplify the build.
Summary by CodeRabbit
New Features
Tests