crates/mdk-js/README.md (1)

15-21: Add language specifier to fenced code block.

Per coding guidelines, fenced code blocks should have a language specified. Use text or plaintext for directory listings.

📝 Suggested fix

-```
+```text
 src/          # JS source
 native/       # prebuilt libmdk per platform
   linux-x86_64/libmdk.so
   macos-aarch64/libmdk.dylib
   windows-x86_64/mdk.dll

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @crates/mdk-js/README.md around lines 15 - 21, Update the fenced code block
in the README.md directory listing to include a language specifier (use "text"
or "plaintext") so the block starts with text instead of ; locate the
block shown under the directory listing (the triple-backtick fenced block
containing "src/ # JS source" and the native/ lines) and replace the opening
fence accordingly to comply with the coding guideline.

</details>

</blockquote></details>
<details>
<summary>crates/mdk-zig/README.md (1)</summary><blockquote>

`66-82`: **Consider documenting single-threaded usage requirement.**

Based on learnings from the mdk-uniffi crate, MDK instances should be used in a single-threaded context only. This limitation should be mentioned in the documentation to prevent misuse in concurrent Zig applications.



<details>
<summary>📝 Suggested addition after line 82</summary>

```diff
 // Always call deinit when done
 defer m.deinit();
 ```
+
+> ⚠️ **Thread Safety**: The `Mdk` instance is not thread-safe and should only be used from a single thread.
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-zig/README.md` around lines 66 - 82, Add a short note in the
README near the Mdk usage examples that the Mdk instance is not thread-safe and
must be used from a single thread; update the section containing the example
calls (Mdk.init, Mdk.initWithKey, Mdk.initUnencrypted) and the reminder to call
m.deinit() to include a warning like "The Mdk instance is not thread-safe and
should only be used from a single thread" so consumers know to avoid sharing Mdk
across threads.
```

</details>

</blockquote></details>
<details>
<summary>crates/mdk-js/src/ffi_bun.js (1)</summary><blockquote>

`119-129`: **Potential precision loss when converting pointer to `Number`.**

`Number(buf[0])` converts a `BigUint64` to a JavaScript `Number`. On 64-bit systems with addresses above `Number.MAX_SAFE_INTEGER` (2^53), this could lose precision and produce incorrect pointer values. While most user-space addresses are below this threshold, it's a latent risk.



Consider keeping the value as `BigInt` if Bun's FFI layer supports it, or document the limitation.

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-js/src/ffi_bun.js` around lines 119 - 129, The read() function in
allocOutPtr (using BigUint64Array buf) converts buf[0] to a JavaScript Number
which can lose precision for 64-bit pointers; change read() to return the raw
BigInt (e.g., return buf[0]) instead of Number(buf[0]) and update any callers of
allocOutPtr().read() to accept a BigInt pointer (or, if the surrounding Bun FFI
cannot handle BigInt, add a clear comment documenting the precision limitation
and assert/guard for addresses <= Number.MAX_SAFE_INTEGER). Reference:
allocOutPtr, buf, read, BigUint64Array, ptr.
```

</details>

</blockquote></details>
<details>
<summary>crates/mdk-cbindings/src/media.rs (1)</summary><blockquote>

`79-90`: **Consider simplifying `.as_ref().clone()` to `.to_vec()` or similar.**

Line 80 uses `prepared.encrypted_data.as_ref().clone()`, which is an unusual pattern. If `encrypted_data` is already a `Vec<u8>` or implements `Clone`, a simpler pattern would be more idiomatic.



However, if `encrypted_data` is a wrapper type where `as_ref()` returns `&Vec<u8>`, this pattern is correct.

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-cbindings/src/media.rs` around lines 79 - 90, The field assignment
for PreparedImageJson.encrypted_data uses the odd pattern
prepared.encrypted_data.as_ref().clone(); replace it with a clearer conversion
depending on the type: if prepared.encrypted_data implements AsRef<[u8]> use
prepared.encrypted_data.as_ref().to_vec() or prepared.encrypted_data.to_vec(),
otherwise use prepared.encrypted_data.clone(); update the assignment to use one
of these idiomatic forms (refer to PreparedImageJson and the
prepared.encrypted_data expression) so the conversion is explicit and readable.
```

</details>

</blockquote></details>
<details>
<summary>crates/mdk-cbindings/src/lib.rs (1)</summary><blockquote>

`11-14`: **Reconsider `#![deny(unsafe_code)]` immediately followed by `#![allow(unsafe_code)]`.**

The current pattern effectively disables the lint crate-wide. Per coding guidelines, crates should use `#![forbid(unsafe_code)]` or `#![deny(unsafe_code)]`. For FFI crates where unsafe is necessary, the idiomatic approach is:

1. Keep `#![deny(unsafe_code)]` at crate root
2. Use `#[allow(unsafe_code)]` on individual `extern "C"` functions that require it

This ensures any accidental unsafe code in non-FFI helpers is still flagged.


<details>
<summary>♻️ Suggested approach</summary>

```diff
-#![deny(unsafe_code)]
-// We allow `unsafe` only in explicitly marked blocks — the deny is overridden
-// per-function with `#[allow(unsafe_code)]` where FFI requires it.
-#![allow(unsafe_code)]
+#![deny(unsafe_code)]
 #![warn(missing_docs)]
```

Then on each FFI function:
```rust
#[allow(unsafe_code)]
#[unsafe(no_mangle)]
pub unsafe extern "C" fn mdk_new(...) -> MdkError {
```
</details>

Based on learnings: "All crates use `#![forbid(unsafe_code)]` or `#![deny(unsafe_code)]` - flag any `unsafe` blocks."

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-cbindings/src/lib.rs` around lines 11 - 14, Remove the crate-level
override that disables the deny (delete `#![allow(unsafe_code)]`) so
`#![deny(unsafe_code)]` is effective, and instead add `#[allow(unsafe_code)]`
attributes directly on each FFI function (the `pub unsafe extern "C"
fn`/`#[no_mangle]` functions such as mdk_new and other extern "C" entry points)
that legitimately require unsafe; this keeps the crate-wide lint while scoping
allowed unsafe to only the specific FFI functions.
```

</details>

</blockquote></details>
<details>
<summary>crates/mdk-js/src/ffi_deno.ts (2)</summary><blockquote>

`27-38`: **TYPE_MAP is missing entries for some NativeType values.**

The `NativeType` union includes `isize`, `f32`, `f64`, `pointer`, and `buffer`, but `TYPE_MAP` only maps a subset. If `symbols.js` uses any unmapped types, `toDenoSymbols` will produce `undefined` entries, causing runtime failures.

Compare with `ffi_bun.js` which includes `i32` but also lacks some types. Ensure all types used in `symbols.js` are covered.

<details>
<summary>♻️ Proposed fix to add missing type mappings</summary>

```diff
 const TYPE_MAP: Record<string, NativeType> = {
   ptr: "pointer",
   cstr: "buffer",
   u8: "u8",
   u16: "u16",
   u32: "u32",
   u64: "u64",
   usize: "usize",
+  isize: "isize",
   bool: "bool",
   i32: "i32",
+  f32: "f32",
+  f64: "f64",
   void: "void",
 };
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-js/src/ffi_deno.ts` around lines 27 - 38, TYPE_MAP is incomplete
and can return undefined for NativeType entries used by toDenoSymbols; update
TYPE_MAP to include mappings for all NativeType variants (isize, f32, f64,
pointer, buffer and any other types present in the NativeType union and
symbols.js) so toDenoSymbols always produces valid Deno FFI types. Locate the
TYPE_MAP constant and add the missing keys with their corresponding Deno-native
values (e.g., map pointer/isize/buffer/f32/f64 appropriately), then run tests or
inspect toDenoSymbols consumers to confirm no undefined entries remain.
```

</details>

---

`66-71`: **Platform detection hardcodes CPU architecture.**

Darwin is assumed to be `aarch64` and Linux/Windows are assumed to be `x86_64`. This will fail on Intel Macs or ARM Linux/Windows systems.

<details>
<summary>♻️ Proposed fix to detect architecture dynamically</summary>

```diff
-  const platform =
-    Deno.build.os === "darwin"
-      ? "macos-aarch64"
-      : Deno.build.os === "windows"
-        ? "windows-x86_64"
-        : "linux-x86_64";
+  const arch = Deno.build.arch === "aarch64" ? "aarch64" : "x86_64";
+  const osName =
+    Deno.build.os === "darwin"
+      ? "macos"
+      : Deno.build.os === "windows"
+        ? "windows"
+        : "linux";
+  const platform = `${osName}-${arch}`;
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-js/src/ffi_deno.ts` around lines 66 - 71, The platform detection
currently hardcodes CPU architecture in the `platform` ternary (using
`Deno.build.os` only); update it to use `Deno.build.arch` as well and construct
the platform string dynamically (e.g., combine `Deno.build.os` and a mapped
`Deno.build.arch` value) so macOS can resolve to `macos-x86_64` or
`macos-aarch64` and Linux/Windows get the correct architecture too; modify the
`platform` expression to consult `Deno.build.arch` (or map its values like
"x86_64" → "x86_64", "aarch64" → "aarch64") and then produce the final strings
used elsewhere in this file (`platform`).
```

</details>

</blockquote></details>
<details>
<summary>crates/mdk-js/src/mdk.js (1)</summary><blockquote>

`297-313`: **Consider caching the ffi() result to avoid repeated calls.**

These methods call `ffi()` twice each. While the overhead is minimal (it's just checking `_ffi`), caching the result would be more consistent with other methods.

<details>
<summary>♻️ Optional: Cache ffi() result</summary>

```diff
   /** Merge pending commit. `@param` {string} mlsGroupId */
   mergePendingCommit(mlsGroupId) {
-    const cGid = ffi().toCString(mlsGroupId);
-    check(ffi().sym.mdk_merge_pending_commit(this.#handle, cGid.ptr));
+    const f = ffi();
+    const cGid = f.toCString(mlsGroupId);
+    check(f.sym.mdk_merge_pending_commit(this.#handle, cGid.ptr));
   }

   /** Clear pending commit. `@param` {string} mlsGroupId */
   clearPendingCommit(mlsGroupId) {
-    const cGid = ffi().toCString(mlsGroupId);
-    check(ffi().sym.mdk_clear_pending_commit(this.#handle, cGid.ptr));
+    const f = ffi();
+    const cGid = f.toCString(mlsGroupId);
+    check(f.sym.mdk_clear_pending_commit(this.#handle, cGid.ptr));
   }

   /** Sync group metadata from MLS. `@param` {string} mlsGroupId */
   syncGroupMetadata(mlsGroupId) {
-    const cGid = ffi().toCString(mlsGroupId);
-    check(ffi().sym.mdk_sync_group_metadata(this.#handle, cGid.ptr));
+    const f = ffi();
+    const cGid = f.toCString(mlsGroupId);
+    check(f.sym.mdk_sync_group_metadata(this.#handle, cGid.ptr));
   }
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-js/src/mdk.js` around lines 297 - 313, The three methods
mergePendingCommit, clearPendingCommit, and syncGroupMetadata call ffi() twice
each; cache the ffi() result at the start of each method (e.g., const f = ffi())
and use f.sym.* for the check calls so you don't call ffi() again, keeping the
existing cGid handling and check(…) usage unchanged.
```

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @crates/mdk-cbindings/CHANGELOG.md:

• Around line 7-19: The Unreleased changelog only contains an "Added" list
  without the required Keep a Changelog sections and missing PR link suffixes;
  update crates/mdk-cbindings/CHANGELOG.md by adding the standard Keep a Changelog
  sections (e.g., Added, Changed, Fixed, Removed as applicable) under "Unreleased"
  and append PR reference links to each bullet in the format
  (#123); ensure each bullet from the
  current Added list (e.g., "Initial C ABI bindings for mdk-core with SQLite
  storage", "33-function API surface mirroring mdk-uniffi exactly", "Opaque
  MdkHandle...", "JSON serialization...", "Thread-local error message
  storage...", "Panic safety via catch_unwind...", "Null pointer guards...",
  "mdk_string_free...", "cbindgen-based header generation (include/mdk.h)")
  has a corresponding PR number/link and move any non-Added items into their
  appropriate sections per the project changelog guidelines.

In @crates/mdk-cbindings/README.md:

• Around line 17-22: Update the fenced code block in the README.md that lists
  the generated artifacts to include a language specifier (e.g., change the
  opening "" to "text"); locate the block containing the lines
  "include/mdk.h", "lib/linux-x86_64/libmdk.so", "lib/macos-aarch64/libmdk.dylib",
  and "lib/windows-x86_64/mdk.dll" and add the specifier so the block starts with
  "```text" to satisfy markdown linting rules.

In @crates/mdk-cbindings/src/types.rs:

• Around line 67-84: The ffi_catch wrapper currently extracts and forwards panic
  payload content to set_last_error, risking sensitive data leakage; change
  ffi_catch (the function using std::panic::catch_unwind) so that on Err(payload)
  it does NOT include the payload text in set_last_error but records a generic
  message (e.g., "Internal panic occurred") and optionally logs the payload
  locally (not via set_last_error) for debugging; ensure the function still
  returns MdkError::Mdk and keep the generic message in mdk_last_error_message()
  instead of any panic payload.

In @crates/mdk-cbindings/src/welcomes.rs:

• Around line 60-66: The Welcome objects returned by to_json(&welcomes) expose
  sensitive identifiers; update the Welcome struct (in
  crates/mdk-storage-traits/src/welcomes/types.rs) to prevent serialization of the
  mls_group_id and nostr_group_id fields by annotating both fields with
  #[serde(skip_serializing)], then rebuild and ensure callers (e.g., the
  get_pending_welcomes path used by lock_handle(...).get_pending_welcomes and the
  to_json call) still compile and return JSON without those fields.

In @crates/mdk-js/src/ffi_bun.js:

• Around line 41-46: The platform mapping in the const platform block
  incorrectly assumes darwin is always macos-aarch64; update the detection to also
  check process.arch (e.g., process.arch === "arm64" vs "x64") and map darwin to
  "macos-aarch64" for arm64 and "macos-x86_64" (or "macos-x86-64" matching your
  build names) for x86_64 so the platform constant correctly reflects Intel vs
  Apple Silicon Macs; adjust the ternary/branch around process.platform ===
  "darwin" in the platform variable to use process.arch for the darwin case.

---

Outside diff comments:
In @.github/workflows/package-mdk-bindings.yml:

• Line 611: The workflow contains an inconsistent checkout action — the step
  currently using actions/checkout@v6 should be changed to actions/checkout@v4 to
  match the other checkout steps; locate the step that references
  actions/checkout@v6 and update the version tag to @v4 so all checkout uses are
  standardized (ensure no other steps reference @v6).

---

Nitpick comments:
In @crates/mdk-cbindings/src/lib.rs:

• Around line 11-14: Remove the crate-level override that disables the deny
  (delete #![allow(unsafe_code)]) so #![deny(unsafe_code)] is effective, and
  instead add #[allow(unsafe_code)] attributes directly on each FFI function
  (the pub unsafe extern "C" fn/#[no_mangle] functions such as mdk_new and
  other extern "C" entry points) that legitimately require unsafe; this keeps the
  crate-wide lint while scoping allowed unsafe to only the specific FFI functions.

In @crates/mdk-cbindings/src/media.rs:

• Around line 79-90: The field assignment for PreparedImageJson.encrypted_data
  uses the odd pattern prepared.encrypted_data.as_ref().clone(); replace it with a
  clearer conversion depending on the type: if prepared.encrypted_data implements
  AsRef<[u8]> use prepared.encrypted_data.as_ref().to_vec() or
  prepared.encrypted_data.to_vec(), otherwise use prepared.encrypted_data.clone();
  update the assignment to use one of these idiomatic forms (refer to
  PreparedImageJson and the prepared.encrypted_data expression) so the conversion
  is explicit and readable.

In @crates/mdk-js/README.md:

• Around line 15-21: Update the fenced code block in the README.md directory
  listing to include a language specifier (use "text" or "plaintext") so the block
  starts with text instead of ; locate the block shown under the directory
  listing (the triple-backtick fenced block containing "src/ # JS source" and the
  native/ lines) and replace the opening fence accordingly to comply with the
  coding guideline.

In @crates/mdk-js/src/ffi_bun.js:

• Around line 119-129: The read() function in allocOutPtr (using BigUint64Array
  buf) converts buf[0] to a JavaScript Number which can lose precision for 64-bit
  pointers; change read() to return the raw BigInt (e.g., return buf[0]) instead
  of Number(buf[0]) and update any callers of allocOutPtr().read() to accept a
  BigInt pointer (or, if the surrounding Bun FFI cannot handle BigInt, add a clear
  comment documenting the precision limitation and assert/guard for addresses <=
  Number.MAX_SAFE_INTEGER). Reference: allocOutPtr, buf, read, BigUint64Array,
  ptr.

In @crates/mdk-js/src/ffi_deno.ts:

• Around line 27-38: TYPE_MAP is incomplete and can return undefined for
  NativeType entries used by toDenoSymbols; update TYPE_MAP to include mappings
  for all NativeType variants (isize, f32, f64, pointer, buffer and any other
  types present in the NativeType union and symbols.js) so toDenoSymbols always
  produces valid Deno FFI types. Locate the TYPE_MAP constant and add the missing
  keys with their corresponding Deno-native values (e.g., map
  pointer/isize/buffer/f32/f64 appropriately), then run tests or inspect
  toDenoSymbols consumers to confirm no undefined entries remain.
• Around line 66-71: The platform detection currently hardcodes CPU architecture
  in the platform ternary (using Deno.build.os only); update it to use
  Deno.build.arch as well and construct the platform string dynamically (e.g.,
  combine Deno.build.os and a mapped Deno.build.arch value) so macOS can
  resolve to macos-x86_64 or macos-aarch64 and Linux/Windows get the correct
  architecture too; modify the platform expression to consult Deno.build.arch
  (or map its values like "x86_64" → "x86_64", "aarch64" → "aarch64") and then
  produce the final strings used elsewhere in this file (platform).

In @crates/mdk-js/src/mdk.js:

• Around line 297-313: The three methods mergePendingCommit, clearPendingCommit,
  and syncGroupMetadata call ffi() twice each; cache the ffi() result at the start
  of each method (e.g., const f = ffi()) and use f.sym.* for the check calls so
  you don't call ffi() again, keeping the existing cGid handling and check(…)
  usage unchanged.

In @crates/mdk-zig/README.md:

• Around line 66-82: Add a short note in the README near the Mdk usage examples
  that the Mdk instance is not thread-safe and must be used from a single thread;
  update the section containing the example calls (Mdk.init, Mdk.initWithKey,
  Mdk.initUnencrypted) and the reminder to call m.deinit() to include a warning
  like "The Mdk instance is not thread-safe and should only be used from a single
  thread" so consumers know to avoid sharing Mdk across threads.

</details>

---

<details>
<summary>ℹ️ Review info</summary>

**Configuration used**: Path: .coderabbit.yaml

**Review profile**: CHILL

**Plan**: Pro

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 0fbb6b4d6364becc7b8a8bf230c0cf88b2ebe7c0 and 57a1c9cc0942b9ee9afa6821120a3dacf48219c4.

</details>

<details>
<summary>⛔ Files ignored due to path filters (1)</summary>

* `Cargo.lock` is excluded by `!**/*.lock`

</details>

<details>
<summary>📒 Files selected for processing (30)</summary>

* `.github/workflows/package-mdk-bindings.yml`
* `.gitignore`
* `Cargo.toml`
* `crates/mdk-cbindings/CHANGELOG.md`
* `crates/mdk-cbindings/Cargo.toml`
* `crates/mdk-cbindings/README.md`
* `crates/mdk-cbindings/build.rs`
* `crates/mdk-cbindings/cbindgen.toml`
* `crates/mdk-cbindings/include/mdk.h`
* `crates/mdk-cbindings/src/error.rs`
* `crates/mdk-cbindings/src/free.rs`
* `crates/mdk-cbindings/src/groups.rs`
* `crates/mdk-cbindings/src/key_packages.rs`
* `crates/mdk-cbindings/src/lib.rs`
* `crates/mdk-cbindings/src/media.rs`
* `crates/mdk-cbindings/src/messages.rs`
* `crates/mdk-cbindings/src/types.rs`
* `crates/mdk-cbindings/src/welcomes.rs`
* `crates/mdk-js/README.md`
* `crates/mdk-js/src/ffi_bun.js`
* `crates/mdk-js/src/ffi_deno.ts`
* `crates/mdk-js/src/index.js`
* `crates/mdk-js/src/mdk.js`
* `crates/mdk-js/src/mod.ts`
* `crates/mdk-js/src/symbols.js`
* `crates/mdk-zig/README.md`
* `crates/mdk-zig/build.zig`
* `crates/mdk-zig/build.zig.zon`
* `crates/mdk-zig/src/c.zig`
* `crates/mdk-zig/src/mdk.zig`

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

crates/mdk-js/src/ffi_deno.ts (1)

64-64: Windows pathname from URL includes leading slash, but Deno APIs handle this transparently.

new URL(".", import.meta.url).pathname returns /C:/Users/... on Windows per the URL standard. While Deno's file APIs like Deno.dlopen handle this format correctly in modern versions, the path is technically not in standard Windows format.

[optional_refactor]

💡 Optional: Use standard Windows path format for clarity

-  const pkgRoot = new URL(".", import.meta.url).pathname;
+  let pkgRoot = new URL(".", import.meta.url).pathname;
+  // Remove leading slash on Windows paths for standard format
+  if (Deno.build.os === "windows" && pkgRoot.match(/^\/[A-Za-z]:\//)) {
+    pkgRoot = pkgRoot.slice(1);
+  }

This improves path readability without being functionally necessary, as Deno already normalizes these paths internally before system calls.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-js/src/ffi_deno.ts` at line 64, The pkgRoot assignment uses new
URL(".", import.meta.url).pathname which yields a POSIX-style leading-slash
Windows path; change this to produce a platform-native path by using the
std/path helper (fromFileUrl) or otherwise converting the file URL to a native
Windows path when Deno.build.os === "windows"; update the pkgRoot initialization
in ffi_deno.ts (the const pkgRoot binding) to call fromFileUrl(new URL(".",
import.meta.url)) so pkgRoot is in standard Windows format on Windows and
unchanged on POSIX.

crates/mdk-zig/src/mdk.zig (1)

269-273: Missing null-check on output pointers for operations returning CString.

While constructors validate output pointers, operations like createKeyPackage, createKeyPackageWithOptions, and parseKeyPackage directly return CString{ .ptr = out } without checking if out is null after the C call succeeds. This is inconsistent with the constructor pattern and could lead to crashes if the C layer has bugs.

Consider adding validation for consistency:

♻️ Example fix for createKeyPackage

         var out: [*c]u8 = null;
         const h = try self.liveHandle();
         try check(raw.mdk_create_key_package(h, c_pk, c_relays, &out));
-        return CString{ .ptr = out };
+        if (out == null) return Error.NullPointer;
+        return CString{ .ptr = out };

Also applies to: 287-291, 301-305

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-zig/src/mdk.zig` around lines 269 - 273, The functions
createKeyPackage, createKeyPackageWithOptions, and parseKeyPackage return
CString{ .ptr = out } without verifying that the C-out pointer is non-null;
replicate the constructor pattern by checking that out != null after the
successful raw call (e.g., after raw.mdk_create_key_package/have corresponding
raw calls) and return an appropriate error (or use try) if out is null before
constructing and returning CString; reference the functions
createKeyPackage/createKeyPackageWithOptions/parseKeyPackage, the local variable
out, and the raw.mdk_* calls (and liveHandle()) when adding the null-checks.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

crates/mdk-zig/src/mdk.zig (2)

140-143: ⚠️ Potential issue | 🔴 Critical

Use std.json.stringifyAlloc; std.json.Stringify.valueAlloc is not a valid Zig stdlib call.

Line 142 is a compile blocker with current Zig stdlib API naming.

🛠️ Suggested fix

-    const result = try std.json.Stringify.valueAlloc(allocator, conf, .{ .emit_null_optional_fields = false });
+    const result = try std.json.stringifyAlloc(allocator, conf, .{ .emit_null_optional_fields = false });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-zig/src/mdk.zig` around lines 140 - 143, The call to
std.json.Stringify.valueAlloc in configToJson is invalid with the current Zig
stdlib; replace that call with std.json.stringifyAlloc (using the same allocator
and conf, and preserve the options {.emit_null_optional_fields = false}) so
configToJson compiles; update the invocation inside fn configToJson(allocator:
std.mem.Allocator, cfg: ?Config) to call std.json.stringifyAlloc and return its
result.

---

712-731: ⚠️ Potential issue | 🟠 Major

Validate out_ptr before constructing CBytes.

After Line 718 succeeds, the wrapper still assumes out_ptr is non-null. A defensive null-check prevents creating an invalid owned buffer if the C contract drifts.

🛡️ Suggested hardening

     try check(raw.mdk_decrypt_group_image(
         data.ptr,
         data.len,
         hash_ptr,
         hash_len,
         key,
         32,
         nonce,
         12,
         &out_ptr,
         &out_len,
     ));
+    if (out_ptr == null) return Error.NullPointer;
     return CBytes{ .ptr = out_ptr, .len = out_len };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-zig/src/mdk.zig` around lines 712 - 731, After calling
raw.mdk_decrypt_group_image (in the wrapper that returns CBytes), validate that
out_ptr is not null before constructing and returning CBytes; if out_ptr is null
or out_len is zero treat it as an error (return a Zig error or call the existing
error path) so you don't create an invalid owned buffer—add the nil-check for
out_ptr immediately after the try check and handle it consistently with the
function's error-reporting convention.

crates/mdk-cbindings/src/media.rs (1)

78-82: ⚠️ Potential issue | 🔴 Critical

Remove raw {e} interpolation from warning logs in these FFI crypto paths.

At Line 79, Line 168, and Line 218, tracing::warn!(... {e}) can leak internal details from core errors. Keep the warning text static/sanitized in these paths.

🔐 Patch pattern (apply to all three `map_err` blocks)

-        let prepared = core_prepare(image_bytes, mime_str).map_err(|e| {
-            tracing::warn!("Prepare group image failed: {e}");
+        let prepared = core_prepare(image_bytes, mime_str).map_err(|_| {
+            tracing::warn!("Prepare group image failed");
             error::set_last_error("Prepare group image failed");
             MdkError::Mdk
         })?;

As per coding guidelines: "Never log sensitive identifiers ... in tracing::* macros."

Also applies to: 167-171, 217-221

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-cbindings/src/media.rs` around lines 78 - 82, Remove the raw {e}
interpolation from the three map_err closures and replace it with a
static/sanitized warning message: locate the map_err around core_prepare (the
closure currently calling tracing::warn!("Prepare group image failed: {e}")) and
the two other similar map_err blocks referenced in the comment, and change the
tracing::warn! call to a fixed message like tracing::warn!("Prepare group image
failed") (or an equivalent static text for the other two contexts); keep
error::set_last_error("Prepare group image failed") and returning MdkError::Mdk
unchanged so only the logged text is sanitized while preserving existing error
flow.

crates/mdk-js/src/ffi_deno.ts (2)

47-51: Consider validating unknown types in symbol conversion.

If defs contains a type not present in TYPE_MAP, the mapping silently produces undefined, which may cause confusing runtime errors from Deno.dlopen. A defensive check could improve debuggability.

💡 Suggested defensive check

   for (const [name, def] of Object.entries(defs)) {
+    const parameters = def.params.map((t) => {
+      const mapped = TYPE_MAP[t];
+      if (!mapped) throw new Error(`Unknown FFI type "${t}" in symbol "${name}"`);
+      return mapped;
+    });
+    const result = TYPE_MAP[def.result];
+    if (!result) throw new Error(`Unknown FFI result type "${def.result}" in symbol "${name}"`);
     out[name] = {
-      parameters: def.params.map((t) => TYPE_MAP[t]),
-      result: TYPE_MAP[def.result],
+      parameters,
+      result,
     };
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-js/src/ffi_deno.ts` around lines 47 - 51, Loop over defs and
validate that every type referenced in def.params and def.result exists in
TYPE_MAP before assigning to out[name]; if any mapping yields undefined, throw a
clear Error that includes the symbol name (name) and the unknown type(s). Update
the conversion logic around the loop that builds out[name] (the code iterating
"for (const [name, def] of Object.entries(defs))") to collect unknown types from
def.params and def.result and fail-fast with a descriptive message (so the
Deno.dlopen consumer sees the exact missing type).

---

79-85: Consider adding monorepo sibling paths for dev parity with Bun backend.

The Bun backend includes additional candidate paths for monorepo development (mdk-cbindings/target/...). Adding these would provide consistent developer experience across runtimes.

💡 Suggested additional paths

   const candidates = [
     // Packaged layout
     `${pkgRoot}../native/${platform}/${base}`,
     // Development: cargo build output
     `${pkgRoot}../../../target/release/${base}`,
     `${pkgRoot}../../../target/debug/${base}`,
+    // Monorepo sibling
+    `${pkgRoot}../../mdk-cbindings/target/release/${base}`,
+    `${pkgRoot}../../mdk-cbindings/target/debug/${base}`,
   ];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-js/src/ffi_deno.ts` around lines 79 - 85, The candidates array for
native lookup (variable candidates using pkgRoot, platform, base) is missing
monorepo sibling paths used by the Bun backend; add sibling dev paths that point
to the mdk-cbindings crate build outputs (e.g. sibling
mdk-cbindings/target/release/${base} and mdk-cbindings/target/debug/${base})
alongside the existing cargo target entries so the lookup in the same candidates
array (where candidates is declared) will find monorepo-built artifacts during
development.

crates/mdk-cbindings/include/mdk.h (1)

126-127: Prefer plain C symbol references in header comments over Rust intra-doc links.

Line 126 and Line 439 include Rust-style links (e.g., crate::...) that are not meaningful to C consumers. Consider emitting plain function names in generated comments for portability/readability.

Also applies to: 439-440

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-cbindings/include/mdk.h` around lines 126 - 127, Replace Rust
intra-doc links in the C header comments with plain C symbol names: change
occurrences like `crate::free::mdk_string_free` to just `mdk_string_free` (and
likewise for any other `crate::...` links found around the other comment blocks
such as the ones at lines 439-440) so that the generated comments reference the
C ABI symbols directly and are readable by C consumers.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

crates/mdk-zig/src/mdk.zig (1)

140-143: ⚠️ Potential issue | 🔴 Critical

Use std.json.stringifyAlloc; current API call is invalid.

std.json.Stringify.valueAlloc is not a valid stdlib JSON allocation API and will fail to compile on current Zig stdlib.

✅ Proposed fix

 fn configToJson(allocator: std.mem.Allocator, cfg: ?Config) std.mem.Allocator.Error!?[]const u8 {
     const conf = cfg orelse return null;
-    const result = try std.json.Stringify.valueAlloc(allocator, conf, .{ .emit_null_optional_fields = false });
+    const result = try std.json.stringifyAlloc(allocator, conf, .{ .emit_null_optional_fields = false });
     return result;
 }

#!/bin/bash
set -euo pipefail

# 1) Show current callsite(s) in this wrapper.
rg -n 'std\.json\.Stringify\.valueAlloc|std\.json\.stringifyAlloc' crates/mdk-zig/src/mdk.zig

# 2) Verify stdlib API names from Zig's std/json/stringify.zig.
curl -fsSL https://raw.githubusercontent.com/ziglang/zig/main/lib/std/json/stringify.zig \
  | rg -n 'pub fn stringifyAlloc|Stringify\.valueAlloc'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-zig/src/mdk.zig` around lines 140 - 143, The call to the
non-existent std.json.Stringify.valueAlloc in function configToJson is invalid;
replace it with the correct API std.json.stringifyAlloc by calling
std.json.stringifyAlloc(allocator, conf, .{ .emit_null_optional_fields = false
}) inside configToJson and return its result (keeping the existing return type
Error!?[]const u8). Update the function body in configToJson to use
std.json.stringifyAlloc and ensure any error propagation (try) and the
null-early-return (cfg orelse return null) remain unchanged.

crates/mdk-cbindings/src/error.rs (1)

157-168: Test name and behavior are mismatched.

from_storage_error_sets_message currently tests invalid_input, not from_storage_error, so it won’t catch regressions in storage-error mapping.

🧪 Suggested cleanup

-    fn from_storage_error_sets_message() {
-        // We can't easily construct a StorageError, but we can test
-        // the simpler helpers
+    fn invalid_input_sets_message() {
+        // Tests invalid_input helper behavior.
         let code = invalid_input("bad input");
         assert_eq!(code, MdkError::InvalidInput);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-cbindings/src/error.rs` around lines 157 - 168, The test named
from_storage_error_sets_message is asserting the behavior of invalid_input
instead of exercising from_storage_error; either rename the test to reflect it
checks invalid_input (e.g., invalid_input_sets_message) or change the test body
to actually call from_storage_error with a constructed StorageError and assert
the returned MdkError and message via from_storage_error and
mdk_last_error_message; update references inside the test
(from_storage_error_sets_message, invalid_input, from_storage_error,
mdk_last_error_message) accordingly so the name and assertions match the
behavior being tested.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

crates/mdk-zig/src/mdk.zig (2)

42-44: ⚠️ Potential issue | 🟡 Minor

Clarify lastErrorMessage cleanup docs to require explicit deinit().

Line 44 still implies scope-based cleanup; this type must be explicitly deinitialized.

📝 Proposed doc fix

 /// Returns `null` if no error has been recorded on this thread (or if the
 /// message has already been consumed).  The returned `CString` must be
-/// allowed to go out of scope (its `deinit` frees the C memory).
+/// explicitly deinitialized (e.g. `defer msg.deinit();`).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-zig/src/mdk.zig` around lines 42 - 44, The docs for
lastErrorMessage incorrectly imply scope-based cleanup; update the comment for
the function/variable named lastErrorMessage to explicitly state that the
returned CString must be explicitly deinit()'d by the caller (no automatic
scope-based free), e.g., replace wording that says it "must be allowed to go out
of scope" with a sentence that callers are required to call CString.deinit() to
free the C memory after use and that null indicates no error or already-consumed
message.

---

141-144: ⚠️ Potential issue | 🔴 Critical

Replace invalid JSON API call with std.json.stringifyAlloc.

Line 143 uses a non-existent stdlib API and will fail compilation.

🔧 Proposed fix

 fn configToJson(allocator: std.mem.Allocator, cfg: ?Config) std.mem.Allocator.Error!?[]const u8 {
     const conf = cfg orelse return null;
-    const result = try std.json.Stringify.valueAlloc(allocator, conf, .{ .emit_null_optional_fields = false });
+    const result = try std.json.stringifyAlloc(allocator, conf, .{ .emit_null_optional_fields = false });
     return result;
 }

#!/bin/bash
set -euo pipefail

# Verify current API usage in Zig files.
rg -n --type zig 'std\.json\.Stringify\.valueAlloc|std\.json\.stringifyAlloc'

# Inspect the exact affected block.
sed -n '141,145p' crates/mdk-zig/src/mdk.zig

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-zig/src/mdk.zig` around lines 141 - 144, The call to a
non-existent std.json.Stringify.valueAlloc in function configToJson causes
compilation failure; replace it with the correct Zig stdlib API
std.json.stringifyAlloc and pass the same allocator, value, and options (e.g.,
.{ .emit_null_optional_fields = false }) so the function returns the allocated
JSON slice as before. Update the invocation inside configToJson to use
std.json.stringifyAlloc(allocator, conf, .{ .emit_null_optional_fields = false
}) and keep the existing error handling and return path.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

crates/mdk-zig/src/mdk.zig (1)

141-145: ⚠️ Potential issue | 🔴 Critical

std.json.Stringify.valueAlloc is not a valid Zig standard library API.

The Zig standard library provides std.json.stringifyAlloc for JSON serialization, not std.json.Stringify.valueAlloc. This will cause a compilation error.

🐛 Proposed fix

 fn configToJson(allocator: std.mem.Allocator, cfg: ?Config) std.mem.Allocator.Error!?[]const u8 {
     const conf = cfg orelse return null;
-    const result = try std.json.Stringify.valueAlloc(allocator, conf, .{ .emit_null_optional_fields = false });
+    const result = try std.json.stringifyAlloc(allocator, conf, .{ .emit_null_optional_fields = false });
     return result;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-zig/src/mdk.zig` around lines 141 - 145, The call in function
configToJson uses a non-existent API std.json.Stringify.valueAlloc; replace it
with the correct Zig stdlib function std.json.stringifyAlloc and adapt the call
signature to match stringifyAlloc (pass allocator and the value, and any options
via std.json.StringifyOptions if needed) so configToJson compiles; update any
option names to the types expected by std.json.stringifyAlloc and ensure the
return/errset stays consistent with configToJson's signature.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

crates/mdk-js/src/mdk.js (1)

559-573: ⚠️ Potential issue | 🔴 Critical

Avoid unconditional out-buffer cleanup in decryptGroupImage catch path.

At Line 571, out.readAndFree() runs even if argument evaluation throws before the native call executes (for example, f.bufferPtr(data)), which can free unread/uninitialized out pointers.

🔧 Proposed fix

 export function decryptGroupImage(data, expectedHash, key, nonce) {
@@
   const f = ffi();
   const out = f.allocOutBytes();
+  let called = false;
   try {
-    check(f.sym.mdk_decrypt_group_image(
+    const code = f.sym.mdk_decrypt_group_image(
       f.bufferPtr(data), data.length,
       expectedHash ? f.bufferPtr(expectedHash) : f.nullptr,
       expectedHash ? expectedHash.length : 0,
       f.bufferPtr(key), key.length,
       f.bufferPtr(nonce), nonce.length,
       out.ptrPtr, out.lenPtr,
-    ));
+    );
+    called = true;
+    check(code);
     return out.readAndFree();
   } catch (e) {
-    out.readAndFree();
+    if (called) out.readAndFree();
     throw e;
   }
 }

#!/bin/bash
set -euo pipefail

echo "=== Inspect allocOutBytes/readAndFree contracts in backends ==="
rg -n -C4 'allocOutBytes|readAndFree|mdk_bytes_free|ptrPtr|lenPtr' crates/mdk-js/src/ffi_bun.js crates/mdk-js/src/ffi_deno.ts

echo
echo "=== Inspect decryptGroupImage error-path cleanup ==="
sed -n '548,575p' crates/mdk-js/src/mdk.js

Expected verification result: if backend readAndFree() assumes pointers are written by native code, unconditional cleanup on pre-call exceptions is unsafe and must stay guarded.

Based on learnings: MDK handles cryptographic operations so be careful with key material. Review security implications of any changes to cryptographic code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-js/src/mdk.js` around lines 559 - 573, The catch path
unconditionally calls out.readAndFree() even if argument evaluation (e.g.,
f.bufferPtr(data)) threw before the native call wrote the out pointers; change
decryptGroupImage so the out buffer is only cleaned up when the native call
actually ran and populated the pointers: allocate out as now, wrap only the
native call+readAndFree sequence so that after
check(f.sym.mdk_decrypt_group_image(...)) returns successfully you call
out.readAndFree(), and in the catch block only call out.readAndFree() if a
boolean flag (e.g., nativeCalled or outPopulated) was set immediately after the
native call succeeded; reference symbols: decryptGroupImage, allocOutBytes,
out.readAndFree, f.sym.mdk_decrypt_group_image, f.bufferPtr.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

.github/workflows/package-mdk-bindings.yml (2)

701-714: Consider documenting the OS-to-architecture mapping assumption.

The matrix implicitly assumes macos-latest produces aarch64 binaries. While this is currently correct, adding a brief comment documenting this assumption would help future maintainers. Additionally, Linux ARM64 (aarch64) builds are not included—this may be intentional for initial release but worth tracking if ARM Linux support is desired.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/package-mdk-bindings.yml around lines 701 - 714, Add a
short inline comment above the build-ffi-native matrix explaining the
OS-to-architecture assumptions (e.g., that macos-latest targets aarch64 while
macos-15-intel targets x86_64) so future maintainers understand the mapping, and
optionally note that Linux ARM64 (aarch64) is not currently included; update the
matrix declaration near the build-ffi-native job and the matrix include entries
(matrix.include, macos-latest, macos-15-intel, ubuntu-latest, windows-latest) to
reflect this comment and/or add a TODO entry for adding an ubuntu-arm64
(aarch64) row if ARM Linux support should be tracked.

---

773-782: Consider extracting version extraction into a reusable workflow or composite action.

The version extraction logic is duplicated across publish-cbindings, publish-zig, and publish-js jobs. While the current approach is clear and self-contained, extracting this into a composite action would reduce duplication and ensure consistency if the extraction logic needs to change.

Also applies to: 871-880, 972-981

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/package-mdk-bindings.yml around lines 773 - 782, The
"Extract version" step (id: cargo_version) duplicates the same shell extraction
logic across the publish-cbindings, publish-zig, and publish-js jobs; refactor
this by moving the grep/sed extraction into a reusable composite action or a
reusable workflow that exposes the extracted VERSION as an output, then replace
each inlined "Extract version" step with a call to that composite (or uses
workflow_call) so all jobs consume the single source-of-truth output and avoid
duplication.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro