Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new QuickJS-based WASM plugin crate (nix-wasm-plugin-quickjs), Nix flake build rules, tests, and small API extensions in nix-wasm-rust to support WASI argument handling and value return. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Nix
participant WASI_Plugin as "WASM Plugin\n(nix-wasm-plugin-quickjs)"
participant QuickJS
participant nix_wasm as "nix-wasm-rust API"
Nix->>WASI_Plugin: launch (_start) with wasi_arg (JS code)
WASI_Plugin->>nix_wasm: nix_wasm_init_v1()
WASI_Plugin->>QuickJS: create Runtime & Context
WASI_Plugin->>QuickJS: eval(JS code) -> JsValue
QuickJS-->>WASI_Plugin: JsValue result
WASI_Plugin->>nix_wasm: convert JsValue -> Value
WASI_Plugin->>nix_wasm: return_to_nix(Value) (exit with value)
nix_wasm-->>Nix: plugin result (Value)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 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 `@nix-wasm-plugin-quickjs/src/lib.rs`:
- Around line 10-53: The js_value_to_nix function currently recurses unbounded
and can overflow on deeply nested or cyclic JS values; modify js_value_to_nix to
accept a recursion depth counter (e.g., depth: usize) and a cycle-detection
context (e.g., a mutable HashSet or pointer-ID set) and bail with a clear error
when depth exceeds a safe max or when a JS object/array identity is seen twice;
update all recursive calls (including those in the array handling loop and the
object props loop) to pass depth+1 and the same context, and ensure public call
sites initialize depth=0 and an empty context before invoking js_value_to_nix so
Value::make_list and Value::make_attrset only receive fully validated,
non-cyclic conversions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockflake.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlflake.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.rs
Comment on lines
+10
to
+53
| 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 recursion guard for nested/cyclic JS values.
Lines [10-53] recurse without depth/cycle limits; deeply nested or self-referential objects can overflow the stack instead of failing cleanly.
Proposed hardening patch
-fn js_value_to_nix(value: JsValue) -> Value {
+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 nesting depth exceeded");
+ }
if value.is_null() || value.is_undefined() {
return Value::make_null();
}
@@
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));
+ items.push(js_value_to_nix_with_depth(entry, depth + 1));
}
return Value::make_list(&items);
}
@@
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)));
+ 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 - 53, The js_value_to_nix
function currently recurses unbounded and can overflow on deeply nested or
cyclic JS values; modify js_value_to_nix to accept a recursion depth counter
(e.g., depth: usize) and a cycle-detection context (e.g., a mutable HashSet or
pointer-ID set) and bail with a clear error when depth exceeds a safe max or
when a JS object/array identity is seen twice; update all recursive calls
(including those in the array handling loop and the object props loop) to pass
depth+1 and the same context, and ensure public call sites initialize depth=0
and an empty context before invoking js_value_to_nix so Value::make_list and
Value::make_attrset only receive fully validated, non-cyclic conversions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
flake.nix (2)
71-80: Vendor merge order could cause subtle issues.The stdlibVendor is copied after workspaceVendor (line 77), potentially overwriting crates. While the Cargo.lock and config.toml are restored from workspaceVendor afterward (lines 78-79), any crate source directories with the same name would be overwritten by stdlibVendor's versions.
If this is intentional (stdlib crates should take precedence), consider adding a comment. Otherwise, verify no crate collisions exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 71 - 80, The current cargoVendor runCommand copies workspaceVendor first then stdlibVendor, which lets stdlibVendor overwrite workspace crates; either switch the copy order so ${stdlibVendor} is copied first and then ${workspaceVendor} (so workspaceVendor takes precedence), or explicitly document the intended precedence with a comment; update references to ensure Cargo.lock and .cargo/config.toml still come from workspaceVendor and verify no crate-name collisions remain if you keep the current order.
52-60: Hardcoded Clang version 19 may cause brittleness.The paths
$out/lib/clang/19and references to clang 19 are hardcoded. When nixpkgs updates LLVM/Clang to a newer version, this will break.Consider deriving the version dynamically or documenting this coupling.
♻️ Suggested approach
+ clangVersion = lib.versions.major llvmPackages.clang.version; wasiSdk = runCommand "wasi-sdk-compat" { } '' - mkdir -p $out/bin $out/lib/clang/19 $out/share + mkdir -p $out/bin $out/lib/clang/${clangVersion} $out/share ln -s ${wasiCc}/bin/wasm32-unknown-wasi-clang $out/bin/clang # ... rest of symlinks ... - ln -s ${wasiCc}/resource-root/include $out/lib/clang/19/include + ln -s ${wasiCc}/resource-root/include $out/lib/clang/${clangVersion}/include ln -s ${wasiSysroot} $out/share/wasi-sysroot '';Then update lines 115-116 to use the variable as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 52 - 60, Replace the hardcoded "19" in the clang include path ($out/lib/clang/19) with a derived clangVersion variable: compute clangVersion from the wasiCc package (e.g., read the directory name under ${wasiCc}/lib/clang or extract from wasiCc metadata with a fallback), then use that variable when creating the symlink to $out/lib/clang/${clangVersion}/include; also update the other places that reference the clang version (the resource-root/include symlink and any later occurrences where clang version is used) to use ${clangVersion} and keep a sensible fallback if detection fails.
🤖 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`:
- Line 26: The flake currently pins a hard-coded attribute rustPackages_1_89
which may not exist in future nixpkgs; update the rustPackages binding so it
first uses pkgs.rustPackages_1_89 when available and otherwise falls back to a
stable/default like pkgs.rustPackages (or use pkgs.rustChannels.stable.*) to
avoid breakage. Locate the rustPackages = pkgs.rustPackages_1_89 line and
replace it with a conditional/fallback that checks pkgs ? rustPackages_1_89 and
selects the alternative when missing; alternatively, switch to using
pkgs.rustChannels or an explicit rust toolchain provider so the flake remains
robust against nixpkgs attribute changes.
---
Nitpick comments:
In `@flake.nix`:
- Around line 71-80: The current cargoVendor runCommand copies workspaceVendor
first then stdlibVendor, which lets stdlibVendor overwrite workspace crates;
either switch the copy order so ${stdlibVendor} is copied first and then
${workspaceVendor} (so workspaceVendor takes precedence), or explicitly document
the intended precedence with a comment; update references to ensure Cargo.lock
and .cargo/config.toml still come from workspaceVendor and verify no crate-name
collisions remain if you keep the current order.
- Around line 52-60: Replace the hardcoded "19" in the clang include path
($out/lib/clang/19) with a derived clangVersion variable: compute clangVersion
from the wasiCc package (e.g., read the directory name under ${wasiCc}/lib/clang
or extract from wasiCc metadata with a fallback), then use that variable when
creating the symlink to $out/lib/clang/${clangVersion}/include; also update the
other places that reference the clang version (the resource-root/include symlink
and any later occurrences where clang version is used) to use ${clangVersion}
and keep a sensible fallback if detection fails.
| nix-wasm-plugin-quickjs = with pkgs; | ||
| let | ||
| quickjsCargoToml = builtins.fromTOML (builtins.readFile ./nix-wasm-plugin-quickjs/Cargo.toml); | ||
| rustPackages = pkgs.rustPackages_1_89; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the flake.nix file content to understand the setup
head -50 flake.nixRepository: DeterminateSystems/nix-wasm-rust
Length of output: 2162
🏁 Script executed:
# Search for rustPackages references and Rust version patterns in nix files
rg "rustPackages|rust.*1\.89" --type nix -B2 -A2Repository: DeterminateSystems/nix-wasm-rust
Length of output: 1142
🏁 Script executed:
# Check if there are any inputs or nixpkgs pinning
rg "inputs\.|nixpkgs" --type nix -B1 -A1Repository: DeterminateSystems/nix-wasm-rust
Length of output: 731
🏁 Script executed:
# Check if flake.lock exists to see the pinned nixpkgs version
cat flake.lock 2>/dev/null | head -100 || echo "flake.lock not found"Repository: DeterminateSystems/nix-wasm-rust
Length of output: 3237
🏁 Script executed:
# Check if there are any other references to Rust version selection or error handling
rg "rustPackages|rustc|rust.* 1\." --type nix -A3 -B3Repository: DeterminateSystems/nix-wasm-rust
Length of output: 2644
🏁 Script executed:
# Check if there's any fallback or version selection logic
rg "if.*then|else|or " --type nix | head -20Repository: DeterminateSystems/nix-wasm-rust
Length of output: 510
Hard-coded Rust version dependency is rigid; ensure it remains compatible with nixpkgs updates.
The reference to rustPackages_1_89 depends on the pinned nixpkgs providing this attribute. While the current flake.lock (Jan 2025) should include Rust 1.89, this creates a maintenance risk if nixpkgs is updated to an older version or if the attribute is removed in future nixpkgs releases. Consider adding version flexibility or explicit handling for version availability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` at line 26, The flake currently pins a hard-coded attribute
rustPackages_1_89 which may not exist in future nixpkgs; update the rustPackages
binding so it first uses pkgs.rustPackages_1_89 when available and otherwise
falls back to a stable/default like pkgs.rustPackages (or use
pkgs.rustChannels.stable.*) to avoid breakage. Locate the rustPackages =
pkgs.rustPackages_1_89 line and replace it with a conditional/fallback that
checks pkgs ? rustPackages_1_89 and selects the alternative when missing;
alternatively, switch to using pkgs.rustChannels or an explicit rust toolchain
provider so the flake remains robust against nixpkgs attribute changes.
Merged
edolstra
closed this pull request by merging all changes
into
DeterminateSystems:main
in
5e87518
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.
Summary by CodeRabbit
New Features
Tests
Chores