flake.nix (1)

7-7: Verify the custom Nix fork dependency.

The flake depends on a custom Nix fork with a wasm branch, suggesting the builtins.wasm functionality used in tests is not yet in upstream Nix. Ensure this is documented in the README so users understand they need this specific Nix version.

Do you want me to help create documentation explaining this dependency?

nix-wasm-plugin-fib/src/lib.rs (1)

7-13: Exponential time complexity in recursive Fibonacci.

The naive recursive implementation has O(2^n) complexity. While this works for the test case (n=33), larger inputs will be extremely slow or may cause stack overflow. For a demo/example plugin this is likely acceptable, but consider noting this limitation or using an iterative approach if real-world usage is expected.

🔎 Optional iterative alternative

-    fn fib2(n: i64) -> i64 {
-        if n < 2 {
-            1
-        } else {
-            fib2(n - 1) + fib2(n - 2)
-        }
-    }
+    fn fib2(n: i64) -> i64 {
+        if n < 2 {
+            return 1;
+        }
+        let (mut a, mut b) = (1i64, 1i64);
+        for _ in 2..=n {
+            (a, b) = (b, a + b);
+        }
+        b
+    }

nix-wasm-plugin-ini/src/lib.rs (1)

8-8: Consider graceful error handling for malformed INI input.

Using expect() will cause a panic if the INI parsing fails, which may abort the WASM module. Since this parses user-provided data (not system initialization), consider returning an error value or propagating the error more gracefully to the caller.

Based on the retrieved learnings, unwraps are acceptable during Nix initialization for catastrophic failures, but this is user input parsing where malformed data is a recoverable scenario.

nix-wasm-plugin-yaml/src/lib.rs (4)

4-32: Consider defensive handling for Yaml::Real conversion.

Line 6: as_f64().expect() should always succeed for Yaml::Real, but the underlying yaml-rust2 implementation returns None if the string can't be parsed as f64. Consider handling this edge case gracefully rather than panicking.

The panic-based error handling for non-string keys (line 22) and unimplemented types (line 30) is acceptable for a plugin context, but be aware these will abort the WASM execution on malformed input.

---

34-47: Missing type validation on input argument.

The function assumes arg is a string and directly calls get_string() without verifying arg.get_type() == Type::String. If called with a non-string value, this could lead to undefined behavior depending on the host's FFI implementation.

🔎 Proposed fix

 #[no_mangle]
 pub extern "C" fn fromYAML(arg: Value) -> Value {
+    assert!(matches!(arg.get_type(), Type::String), "fromYAML expects a string argument");
     let s = arg.get_string();
 
-    let yaml = YamlLoader::load_from_str(&s).unwrap();
+    let yaml = YamlLoader::load_from_str(&s).expect("invalid YAML input");
 
     let mut docs = vec![];

---

49-75: Float serialization may lose precision; error message could be clearer.

1. Line 52: Using format!("{}", v.get_float()) for Yaml::Real may not preserve the original precision or format. Consider using {:?} or a more precise formatter if round-trip fidelity matters.

2. Lines 70-73: The panic message shows the type as a raw integer (v.get_type() as u64), which isn't user-friendly. Consider using Debug formatting if Type implements it.

🔎 Proposed improvement

-        Type::Float => Yaml::Real(format!("{}", v.get_float())),
+        Type::Float => Yaml::Real(v.get_float().to_string()),
...
-        _ => panic!(
-            "Nix type {} cannot be serialized to YAML",
-            v.get_type() as u64
-        ),
+        t => panic!(
+            "Nix type {:?} cannot be serialized to YAML",
+            t
+        ),

---

77-89: Missing type validation on input argument.

Similar to fromYAML, this function assumes arg is a list without validation. Add a type check for robustness.

🔎 Proposed fix

 #[no_mangle]
 pub extern "C" fn toYAML(arg: Value) -> Value {
+    assert!(matches!(arg.get_type(), Type::List), "toYAML expects a list argument");
     let mut out_str = String::new();
 
     for v in arg.get_list() {
         let yaml = to_yaml(v);
         let mut emitter = YamlEmitter::new(&mut out_str);
-        emitter.dump(&yaml).unwrap();
+        emitter.dump(&yaml).expect("failed to emit YAML");
         out_str.push('\n');
     }

nix-wasm-rust/src/lib.rs (1)

12-30: Address FIXME: externref consideration.

The FIXME at line 12 notes a potential improvement using WebAssembly's externref type. This would provide better type safety and garbage collection integration for Value references. Current approach using u32 handles works but requires manual lifecycle management on the host side.

Would you like me to open an issue to track the externref migration?

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: RossComputerGuy
Repo: DeterminateSystems/secure-build PR: 32
File: src/commands/scan.rs:139-162
Timestamp: 2025-10-21T19:23:14.234Z
Learning: In the secure-build repository (Rust), unwraps during Nix initialization (NixStore::open, FlakeSettings::new, EvalStateBuilder, etc.) are acceptable because failures indicate catastrophic system conditions like out-of-memory where no recovery is possible.

Learnt from: RossComputerGuy
Repo: DeterminateSystems/secure-build PR: 32
File: flake.nix:175-177
Timestamp: 2025-10-21T17:29:29.023Z
Learning: In Nix flake.nix files using Rust with bindgen, when `rustPlatform.bindgenHook` is included in `nativeBuildInputs`, it automatically sets up `LIBCLANG_PATH` and other bindgen-related environment variables. An explicit `LIBCLANG_PATH` setting in the package derivation is not required, even if it appears in the devShell.

Learnt from: RossComputerGuy
Repo: DeterminateSystems/secure-build PR: 32
File: src/commands/scan.rs:139-162
Timestamp: 2025-10-21T19:23:14.234Z
Learning: In the secure-build repository (Rust), unwraps during Nix initialization (NixStore::open, FlakeSettings::new, EvalStateBuilder, etc.) are acceptable because failures indicate catastrophic system conditions like out-of-memory where no recovery is possible.

.github/workflows/ci.yml (1)

18-19: Update to actions/checkout@v4.

The v3 runner is deprecated and may not work on newer GitHub Actions infrastructure.

🔎 Proposed fix

-        uses: actions/checkout@v3
+        uses: actions/checkout@v4

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: RossComputerGuy
Repo: DeterminateSystems/secure-build PR: 32
File: src/commands/scan.rs:139-162
Timestamp: 2025-10-21T19:23:14.234Z
Learning: In the secure-build repository (Rust), unwraps during Nix initialization (NixStore::open, FlakeSettings::new, EvalStateBuilder, etc.) are acceptable because failures indicate catastrophic system conditions like out-of-memory where no recovery is possible.

Learnt from: RossComputerGuy
Repo: DeterminateSystems/secure-build PR: 32
File: flake.nix:175-177
Timestamp: 2025-10-21T17:29:29.023Z
Learning: In Nix flake.nix files using Rust with bindgen, when `rustPlatform.bindgenHook` is included in `nativeBuildInputs`, it automatically sets up `LIBCLANG_PATH` and other bindgen-related environment variables. An explicit `LIBCLANG_PATH` setting in the package derivation is not required, even if it appears in the devShell.

.github/workflows/ci.yml (1)

21-22: Fix syntax error in Install Nix step.

Line 22 has an extra leading dash, creating two malformed steps instead of one valid step. This will cause the workflow to fail validation.

🔎 Proposed fix

       - name: Install Nix
-      - uses: DeterminateSystems/determinate-nix-action@main
+        uses: DeterminateSystems/determinate-nix-action@main

nix-wasm-rust/src/lib.rs (2)

122-134: Use mutable pointer for copy_list destination.

The buffer res is written to by the FFI function but is declared immutable and passed via as_ptr(). This should use mut and as_mut_ptr() for correctness.

🔎 Proposed fix

     pub fn get_list(&self) -> Vec<Value> {
         let len = self.get_list_length();
-        let res: Vec<Value> = vec![Value(0); len];
+        let mut res: Vec<Value> = vec![Value(0); len];
         extern "C" {
             fn copy_list(value: ValueId, ptr: u32, len: usize);
         }
         unsafe {
             if len > 0 {
-                copy_list(self.0, res.as_ptr() as u32, len);
+                copy_list(self.0, res.as_mut_ptr() as u32, len);
             }
         }
         res
     }

---

150-175: Use mutable pointers for copy_attrset and copy_attrname destinations.

Both attrs (line 157) and buf (line 165) are written to by FFI functions but are declared immutable. This causes undefined behavior under Rust's aliasing model.

🔎 Proposed fix

     pub fn get_attrset(&self) -> BTreeMap<String, Value> {
         extern "C" {
             fn get_attrset_length(value: ValueId) -> usize;
             fn copy_attrset(value: ValueId, ptr: u32, len: usize);
             fn copy_attrname(value: ValueId, attr_idx: usize, ptr: u32, len: usize);
         }
         let len = unsafe { get_attrset_length(self.0) };
-        let attrs: Vec<(ValueId, usize)> = vec![(0, 0); len];
+        let mut attrs: Vec<(ValueId, usize)> = vec![(0, 0); len];
         if len > 0 {
             unsafe {
-                copy_attrset(self.0, attrs.as_ptr() as u32, len);
+                copy_attrset(self.0, attrs.as_mut_ptr() as u32, len);
             }
         }
         let mut res = BTreeMap::new();
         for (attr_idx, (value, attr_len)) in attrs.iter().enumerate() {
-            let buf: Vec<u8> = vec![0; *attr_len];
+            let mut buf: Vec<u8> = vec![0; *attr_len];
             unsafe {
-                copy_attrname(self.0, attr_idx, buf.as_ptr() as u32, *attr_len);
+                copy_attrname(self.0, attr_idx, buf.as_mut_ptr() as u32, *attr_len);
             }
             res.insert(
                 String::from_utf8(buf).expect("Nix attribute name should be UTF-8."),
                 Value(*value),
             );
         }
         res
     }

nix-wasm-plugin-mandelbrot/src/lib.rs (1)

13-16: Consider validating width to prevent overflow from negative values.

If the width attribute is negative, v.get_int() as usize will wrap to a very large value, potentially causing memory exhaustion or panic when allocating the output string.

🔎 Proposed fix

     let width = args
         .get("width")
-        .map(|v| v.get_int() as usize)
+        .map(|v| v.get_int().max(1) as usize)
         .unwrap_or(120);

nix-wasm-rust/src/lib.rs (1)

12-17: Consider adding Copy derive to Value.

Since Value wraps a u32 (which is Copy), deriving Copy would make the API more ergonomic and avoid unnecessary clones in plugin code.

🔎 Proposed change

 #[repr(transparent)]
-#[derive(Clone, Debug)]
+#[derive(Clone, Copy, Debug)]
 pub struct Value(ValueId);

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: RossComputerGuy
Repo: DeterminateSystems/secure-build PR: 32
File: src/commands/scan.rs:139-162
Timestamp: 2025-10-21T19:23:14.234Z
Learning: In the secure-build repository (Rust), unwraps during Nix initialization (NixStore::open, FlakeSettings::new, EvalStateBuilder, etc.) are acceptable because failures indicate catastrophic system conditions like out-of-memory where no recovery is possible.

Learnt from: RossComputerGuy
Repo: DeterminateSystems/secure-build PR: 32
File: flake.nix:175-177
Timestamp: 2025-10-21T17:29:29.023Z
Learning: In Nix flake.nix files using Rust with bindgen, when `rustPlatform.bindgenHook` is included in `nativeBuildInputs`, it automatically sets up `LIBCLANG_PATH` and other bindgen-related environment variables. An explicit `LIBCLANG_PATH` setting in the package derivation is not required, even if it appears in the devShell.

Learnt from: cole-h
Repo: DeterminateSystems/determinate-nixd PR: 427
File: src/config/mod.rs:75-77
Timestamp: 2025-11-05T19:24:31.622Z
Learning: In `src/config/mod.rs` and `src/command/builder/mod.rs`, the `cpu_count` field uses `isize` (not an unsigned integer type) because it must match the Swift FFI boundary where `swift_rs::Int` is used in the `run_builder_vm` function signature in `src/command/builder/macos.rs`.

flake.nix (1)

10-17: LGTM on helper structure; x86_64-darwin concern already noted.

The forAllSystems helper and cargoToml parsing are correctly implemented. The missing x86_64-darwin in supportedSystems was already flagged in a previous review.

flake.nix (3)

32-48: Consider adding runHook calls to custom phases.

Custom buildPhase, checkPhase, and installPhase implementations should call runHook to ensure standard phase hooks are executed. This allows other tools and overlays to inject pre/post logic.

🔎 Proposed fix

-          buildPhase = "cargo build --release";
+          buildPhase = ''
+            runHook preBuild
+            cargo build --release
+            runHook postBuild
+          '';

           checkPhase = ''
+            runHook preCheck
             for i in nix-wasm-plugin-*/tests/*.nix; do
               echo "running test $i..."
               base="$(dirname $i)/$(basename $i .nix)"
               nix eval --store dummy:// --offline --json --show-trace -I plugins=target/wasm32-unknown-unknown/release --impure --file "$i" > "$base.out"
               cmp "$base.exp" "$base.out"
             done
+            runHook postCheck
           '';

           installPhase = ''
+            runHook preInstall
             mkdir -p $out
             for i in target/wasm32-unknown-unknown/release/*.wasm; do
               wasm-opt -O3 -o "$out/$(basename "$i")" "$i"
             done
+            runHook postInstall
           '';

---

62-71: Unused rec keyword.

The rec on line 62 is unnecessary since default doesn't reference other attributes within the same attrset.

🔎 Proposed fix

-      devShells = forAllSystems ({ pkgs, system }: rec {
-        default = with pkgs; self.packages.${system}.default.overrideAttrs (attrs: {
+      devShells = forAllSystems ({ pkgs, system }: {
+        default = with pkgs; self.packages.${system}.default.overrideAttrs (attrs: {

---

73-76: Unused rec keyword; checks structure is correct.

Same as devShells - the rec is unnecessary here. The check correctly runs the package build for CI validation.

🔎 Proposed fix

-      checks = forAllSystems ({ pkgs, system }: rec {
+      checks = forAllSystems ({ pkgs, system }: {
         build = self.packages.${system}.default;
       });

Configuration used: defaults

Review profile: CHILL

Plan: Pro

flake.nix (1)

10-18: The let bindings are well-structured.

Parsing cargoToml for single source of truth on package metadata and the forAllSystems helper pattern are idiomatic Nix practices.

Note: The missing x86_64-darwin in supportedSystems was flagged in a previous review.

flake.nix (4)

6-6: Unused input: flake-schemas is declared but never referenced.

The flake-schemas input is defined but not used anywhere in the outputs. Consider removing it to reduce unnecessary dependencies, or add a schemas output if you intend to use it.

♻️ Suggested fix

   inputs = {
     nixpkgs.follows = "nix/nixpkgs";
-    flake-schemas.url = "https://flakehub.com/f/DeterminateSystems/flake-schemas/*.tar.gz";
     nix.url = "github:DeterminateSystems/nix-src/wasm";
   };

---

32-32: Missing runHook calls in custom phases.

Custom buildPhase, checkPhase, and installPhase should include runHook preBuild/runHook postBuild (and equivalents) to allow users to inject pre/post hooks when overriding the derivation.

♻️ Suggested fix for buildPhase

-          buildPhase = "cargo build --release";
+          buildPhase = ''
+            runHook preBuild
+            cargo build --release
+            runHook postBuild
+          '';

Similar changes should be applied to checkPhase (add runHook preCheck/runHook postCheck) and installPhase (add runHook preInstall/runHook postInstall).

---

64-73: Unnecessary rec keyword.

The rec keyword on line 65 is unused since default doesn't reference any other attributes in this set. Removing it improves clarity.

♻️ Suggested fix

-      devShells = forAllSystems ({ pkgs, system }: rec {
-        default = with pkgs; self.packages.${system}.default.overrideAttrs (attrs: {
+      devShells = forAllSystems ({ pkgs, system }: {
+        default = self.packages.${system}.default.overrideAttrs (attrs: {
           nativeBuildInputs = attrs.nativeBuildInputs ++ [
-            wabt
-            rust-analyzer
-            rustfmt
-            clippy
+            pkgs.wabt
+            pkgs.rust-analyzer
+            pkgs.rustfmt
+            pkgs.clippy
           ];
         });
       });

Note: The with pkgs; was also moved since it's only needed for the list items after removing rec.

---

75-77: Unnecessary rec keyword in checks.

Similar to devShells, the rec keyword is unused here since build doesn't reference itself or other attributes.

♻️ Suggested fix

-      checks = forAllSystems ({ pkgs, system }: rec {
+      checks = forAllSystems ({ pkgs, system }: {
         build = self.packages.${system}.default;
       });

nix-wasm-rust/src/lib.rs (2)

195-200: make_attrset uses ABI‑unstable (&str, Value) in FFI.

Tuples containing &str are not FFI‑safe; #[allow(improper_ctypes)] just suppresses the warning. Use a repr(C) struct with pointer+length and ValueId (and update the host side to match).

🛠️ Proposed fix

 type ValueId = u32;

+#[repr(C)]
+struct AttrEntry {
+    name_ptr: *const u8,
+    name_len: usize,
+    value_id: ValueId,
+}
+
 impl Value {
     pub fn make_attrset(attrs: &[(&str, Value)]) -> Value {
         extern "C" {
-            #[allow(improper_ctypes)]
-            fn make_attrset(ptr: *const (&str, Value), len: usize) -> Value;
+            fn make_attrset(ptr: *const AttrEntry, len: usize) -> Value;
         }
-        unsafe { make_attrset(attrs.as_ptr(), attrs.len()) }
+        let entries: Vec<AttrEntry> = attrs
+            .iter()
+            .map(|(k, v)| AttrEntry {
+                name_ptr: k.as_ptr(),
+                name_len: k.len(),
+                value_id: v.0,
+            })
+            .collect();
+        unsafe { make_attrset(entries.as_ptr(), entries.len()) }
     }

---

203-219: copy_attrset uses tuple layout across FFI.

*mut (ValueId, usize) has no guaranteed C ABI; replace it with a repr(C) struct and adjust the helper/buffers.

🛠️ Proposed fix

+#[repr(C)]
+struct AttrMeta {
+    value_id: ValueId,
+    name_len: usize,
+}
+
-    fn get_attrset_from_attrs(&self, attrs: &[(ValueId, usize)]) -> BTreeMap<String, Value> {
+    fn get_attrset_from_attrs(&self, attrs: &[AttrMeta]) -> BTreeMap<String, Value> {
         extern "C" {
             fn copy_attrname(value: ValueId, attr_idx: usize, ptr: *mut u8, len: usize);
         }
         let mut res = BTreeMap::new();
-        for (attr_idx, (value, attr_len)) in attrs.iter().enumerate() {
-            let mut buf = vec![0; *attr_len];
+        for (attr_idx, entry) in attrs.iter().enumerate() {
+            let mut buf = vec![0; entry.name_len];
             unsafe {
-                copy_attrname(self.0, attr_idx, buf.as_mut_ptr(), *attr_len);
+                copy_attrname(self.0, attr_idx, buf.as_mut_ptr(), entry.name_len);
             }
             res.insert(
                 String::from_utf8(buf).expect("Nix attribute name should be UTF-8."),
-                Value(*value),
+                Value(entry.value_id),
             );
         }
         res
     }

     pub fn get_attrset(&self) -> BTreeMap<String, Value> {
         extern "C" {
-            #[allow(improper_ctypes)]
-            fn copy_attrset(value: ValueId, ptr: *mut (ValueId, usize), max_len: usize) -> usize;
+            fn copy_attrset(value: ValueId, ptr: *mut AttrMeta, max_len: usize) -> usize;
         }
         unsafe {
             // Optimistically call with a small buffer on the stack.
-            let mut buf = [(0, 0); 32];
+            let mut buf = [AttrMeta { value_id: 0, name_len: 0 }; 32];
             let len = copy_attrset(self.0, buf.as_mut_ptr(), buf.len());
             if len > buf.len() {
                 // If it didn't fit, allocate a buffer of the right size.
-                let mut buf = vec![(0, 0); len];
+                let mut buf = vec![AttrMeta { value_id: 0, name_len: 0 }; len];
                 let len2 = copy_attrset(self.0, buf.as_mut_ptr(), buf.len());
                 assert!(len2 == len);
                 self.get_attrset_from_attrs(&buf)
             } else {
                 self.get_attrset_from_attrs(&buf[0..len])
             }
         }
     }

Also applies to: 221-238

nix-wasm-plugin-test/src/lib.rs (1)

4-75: Prevent panics from crossing the FFI boundary.

All exported extern "C" functions can panic via expect/get_*; unwinding across FFI is UB. Wrap each export with catch_unwind (or use extern "C-unwind" if supported) and return a sentinel value.

🛠️ Proposed fix (apply pattern to all exports)

 use nix_wasm_rust::Value;
+use std::panic;

+fn with_unwind_guard<F: FnOnce() -> Value>(f: F) -> Value {
+    panic::catch_unwind(panic::AssertUnwindSafe(f))
+        .unwrap_or_else(|_| Value::make_null())
+}
+
 #[no_mangle]
 pub extern "C" fn range(arg: Value) -> Value {
-    let args = arg.get_attrset();
-
-    let start = args.get("start").map(|v| v.get_int()).unwrap_or(0);
-    let end = args.get("end").map(|v| v.get_int()).unwrap_or(10);
-
-    let mut list = vec![];
-
-    for i in start..=end {
-        list.push(Value::make_int(i));
-    }
-
-    Value::make_list(&list)
+    with_unwind_guard(|| {
+        let args = arg.get_attrset();
+
+        let start = args.get("start").map(|v| v.get_int()).unwrap_or(0);
+        let end = args.get("end").map(|v| v.get_int()).unwrap_or(10);
+
+        let mut list = vec![];
+
+        for i in start..=end {
+            list.push(Value::make_int(i));
+        }
+
+        Value::make_list(&list)
+    })
 }