Skip to content

feat(cffi): handle table for opaque values, wire serialization for Media/PromptAst#3169

Merged
hellovai merged 6 commits intocanaryfrom
feat/cffi-handle-table-wire-serialization
Feb 25, 2026
Merged

feat(cffi): handle table for opaque values, wire serialization for Media/PromptAst#3169
hellovai merged 6 commits intocanaryfrom
feat/cffi-handle-table-wire-serialization

Conversation

@hellovai
Copy link
Contributor

@hellovai hellovai commented Feb 24, 2026

Summary

This PR introduces a handle-based CFFI model for opaque values (Media, PromptAst, Resources, FunctionRef, etc.) and wire-friendly serialization so the playground (WASM ↔ TypeScript) can display results without a shared process. The approach was inspired by .humanlayer/tasks/vbv-wasm-events/2026-02-21-plan.md (WASM event store / injectable sink); we adapted the idea to the CFFI boundary: injectable encoding options (wire vs in-process) and a global handle table instead of per-object method calls.

Changes

  • Handle table (bridge_ctypes): Global table storing Handle, Resource, FunctionRef, and Adt (Media, PromptAst, Collector, Type). Opaque values are keyed by u64 and can be round-tripped as BamlHandle(key, handle_type).
  • Inbound: InboundValue can carry BamlHandle; decode uses handle_table.resolve(key) to produce BexExternalValue.
  • Outbound: HandleTableOptionsfor_wire() (playground/WASM): serialize Media and PromptAst inline as BamlValueMedia / BamlValuePromptAst; for_in_process() (Python): keep them as handles only.
  • Proto: BamlHandle + BamlHandleType in inbound; first-class BamlValueMedia and BamlValuePromptAst in outbound; removed baml_object.proto and baml_object_methods.proto.
  • FFI: clone_handle / release_handle exposed from C, Python, and WASM.
  • Playground: Media and PromptAst renderers in TS; pkg-proto decode/encode for wire types; worker and ExecutionPanel updated to use the new result shape.
  • Stow: bridge_ctypes now depends only on bex_project; Handle is re-exported from bex_project.

Flow diagram

flowchart TB
  subgraph Host["Host (Python / TS)"]
    A[call_function args]
    B[result JSON / proto]
  end

  subgraph CFFI["CFFI boundary"]
    C[call_function]
    D[callbacks / return]
  end

  subgraph Encode["Encode (outbound)"]
    E[BexExternalValue]
    F{HandleTableOptions}
    G[for_wire]
    H[for_in_process]
    G --> I[BamlValueMedia / BamlValuePromptAst inline]
    H --> J[insert into HandleTable → BamlHandle]
    E --> F
    F --> G
    F --> H
  end

  subgraph Decode["Decode (inbound)"]
    K[InboundValue]
    L{BamlHandle?}
    L -->|yes| M[handle_table.resolve]
    L -->|no| N[primitive / list / map / class]
    M --> O[BexExternalValue]
    N --> O
    K --> L
  end

  subgraph Table["Handle table (global)"]
    T[(key → HandleTableValue)]
    T --> P[Handle / Resource / FunctionRef / Adt]
  end

  A --> K
  Decode --> C
  C --> E
  Encode --> D
  D --> B
  J --> T
  M --> T
Loading

Inbound (host → engine): Host sends InboundValue (primitives, containers, or BamlHandle). Decode resolves handles via the global handle table and produces BexExternalValue for the engine.

Outbound (engine → host): Engine produces BexExternalValue. Encode uses HandleTableOptions:

  • Wire (WASM/playground): Media and PromptAst are serialized inline so the other side can render them without shared memory; other opaques go into the table as handles.
  • In-process (Python): All opaques go into the handle table; host uses clone_handle / release_handle for lifecycle.

Testing

  • cargo test --lib in baml_language (and relevant crates).
  • TypeScript: pkg-proto encode/decode tests; playground run flow if available.

Summary by CodeRabbit

  • New Features

    • Handle lifecycle management for opaque resources across FFI/JS/Python boundaries with clone/release support and automatic cleanup.
    • Playground UI: explicit per-call handle tracking and a clear-handles action.
    • Result rendering: new media and prompt-AST renderers for richer output display.
  • Improvements

    • Call/result encoding now returns structured JSON strings and clearer error messages.
    • Safer resource tracking to reduce leaks in long-running sessions.

…yground renderers

Co-authored-by: Cursor <cursoragent@cursor.com>
@vercel
Copy link

vercel bot commented Feb 24, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
beps Ready Ready Preview, Comment Feb 25, 2026 0:17am
promptfiddle Ready Ready Preview, Comment Feb 25, 2026 0:17am

Request Review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 24, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Introduces a global handle table for FFI boundaries and migrates the protobuf/FFI value protocol from Host/CFFI types to new BAML types (InboundValue/BamlOutboundValue/CallFunctionArgs/BamlHandle). Adds handle lifecycle FFI entry points, updates encoding/decoding across Rust/Python/WASM/TS, removes object-based FFI methods, and adds renderers for media and prompt-AST in the playground.

Changes

Cohort / File(s) Summary
Handle Table Infrastructure
baml_language/crates/bridge_ctypes/src/handle_table.rs, baml_language/crates/bridge_ctypes/src/error.rs, baml_language/crates/bridge_ctypes/Cargo.toml
Adds global HANDLE_TABLE, HandleTable API, HandleTableValue and HandleTableOptions; replaces HandleNotSupported with InvalidHandleKey and InternalError; adds workspace dependency bex_resource_types.
Protobuf / Generated Types
baml_language/crates/bridge_ctypes/types/baml/cffi/v1/baml_inbound.proto, .../baml_outbound.proto, .../baml_object.proto, .../baml_object_methods.proto, baml_language/crates/bridge_python/python_src/baml/cffi/v1/*_pb2.py
Replaces Host*/CFFI* schema with Inbound*/Baml* schema: introduces BamlHandle, InboundValue, CallFunctionArgs, CallAck, renames outbound types (CFFI→Baml), removes object-related protos and corresponding generated Python modules.
Value Encode/Decode (Rust)
baml_language/crates/bridge_ctypes/src/value_encode.rs, .../value_decode.rs, .../lib.rs, .../build.rs
Replaces external_to_cffi_valueexternal_to_baml_value (adds HandleTableOptions); host_value_to_externalinbound_to_external requiring &HandleTable; kwargs_to_bex_values gains &HandleTable; removes baml_object proto generation.
FFI Boundary / C layer
baml_language/crates/bridge_cffi/src/ffi/functions.rs, .../callbacks.rs, .../objects.rs, .../handle.rs, .../mod.rs, .../lib.rs
Threads HANDLE_TABLE into kwargs conversion, switches to CallFunctionArgs/CallAck, uses external_to_baml_value; adds clone_handle/release_handle C exports; removes object constructor/method FFI functions and exposes free_buffer.
Public Re-exports & Builtins
baml_language/crates/bex_project/src/lib.rs, baml_language/crates/bex_project/Cargo.toml
Adds baml_builtins dependency and re-exports builtin types (MediaContent, MediaValue, PromptAst, PromptAstSimple); adds Handle to bex_external_types re-exports.
Python bindings & runtime
baml_language/crates/bridge_python/src/handle.rs, .../lib.rs, .../runtime.rs, .../types/collector.rs, baml_language/crates/bridge_python/Cargo.toml
Adds BamlHandle PyO3 class with lifecycle (__copy__, __del__), registers it, switches runtime/collector to CallFunctionArgs and external_to_baml_value(for_in_process()); adds log dependency.
WASM bindings
baml_language/crates/bridge_wasm/src/handle.rs, .../lib.rs
Adds BamlHandle wasm-bindgen wrapper (key, type, clone, toJSON, Drop) and re-exports HANDLE_TABLE + external_to_baml_value; updates call flow to use CallFunctionArgs and wire options.
TypeScript protocol / pkg-proto
typescript2/pkg-proto/src/encode.ts, .../decode.ts, .../types.ts, .../index.ts, .../test/encode-decode.test.ts
Renames inbound types (Host* → Inbound*/CallFunctionArgs), outbound (CFFIValueHolder → BamlOutboundValue), makes decodeCallResult generic with WrapHandleFn to wrap handles, adds handle/media/prompt-AST JS types, and updates tests to use wrapHandle.
Playground worker & runtime port
typescript2/app-promptfiddle/src/playground/baml-lsp-worker.ts, typescript2/pkg-playground/src/ports/WebSocketRuntimePort.ts, typescript2/pkg-playground/src/worker-protocol.ts
Adds per-call liveHandles management, decodes results with decodeCallResult producing JSON string, changes callFunctionResult payload to string, and adds clearHandles message to free JS-side handle wrappers.
Playground UI & renderers
typescript2/pkg-playground/src/ExecutionPanel.tsx, .../ResultDisplay.tsx, .../renderers/Media.tsx, .../renderers/PromptAst.tsx, .../renderers/registerBuiltins.ts
Result rendering refactor: modular ValueRenderer, adds MediaRenderer and PromptAstRenderer, registers renderers, changes execution panel and pending call typing to expect string results and not decode in UI; Clear flows notify clearHandles.
WASM size / CI config & misc
baml_language/.cargo/size-gate.toml, baml_language/.ci/size-gate/wasm32-unknown-unknown.toml, typescript2/package.json
Adjusts WASM size gate thresholds and CI artifact sizes; adds watcher ignore rules for target dirs.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client/WebSocket
    participant Playground as Playground Worker
    participant LSP as LSP Server
    participant Rust as Rust Runtime
    participant HandleTbl as HANDLE_TABLE

    Note over Client,Rust: Call flow with handle resolution & encoding

    Client->>Playground: callFunction (encoded CallFunctionArgs)
    Playground->>LSP: forward call
    LSP->>Rust: decode CallFunctionArgs -> kwargs_to_bex_values(..., &HANDLE_TABLE)
    Rust->>HandleTbl: resolve referenced handles (if any)
    Rust->>Rust: execute function
    Rust->>HandleTbl: insert opaque results -> u64 keys
    Rust->>Rust: external_to_baml_value(result, HandleTableOptions::for_wire())
    Rust-->>LSP: return encoded BamlOutboundValue (base64)
    LSP-->>Playground: relay result (base64)
    Playground->>Playground: decodeCallResult(base64) -> construct JS BamlHandle wrappers
    Playground-->>Client: callFunctionResult (JSON string with handles)
Loading
sequenceDiagram
    participant JS as JavaScript
    participant WASM as WASM Bridge
    participant Rust as Rust Runtime
    participant HandleTbl as HANDLE_TABLE

    JS->>WASM: new BamlHandle(key, type)
    JS->>JS: store handle in liveHandles
    JS->>WASM: handle.cloneHandle()
    WASM->>Rust: HANDLE_TABLE.clone_handle(key)
    Rust->>HandleTbl: clone and return new_key
    Rust-->>WASM: new_key
    WASM-->>JS: BamlHandle(new_key, type)
    JS->>Playground: clearHandles(runIds)
    Playground->>WASM: release JS-side references
    JS->>WASM: GC drop -> WASM calls HANDLE_TABLE.release(key)
    HandleTbl-->>Rust: remove entry
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Suggested labels

rust, feature (small)

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary change: introducing a handle table for opaque values and wire serialization for Media/PromptAst.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/cffi-handle-table-wire-serialization

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Feb 24, 2026

Binary size checks passed

7 passed

Artifact Platform Gzip Baseline Delta Status
bridge_cffi Linux 3.9 MB 4.0 MB -123.8 KB (-3.1%) OK
bridge_cffi-stripped Linux 2.3 MB 2.1 MB +195.4 KB (+9.1%) OK
bridge_cffi macOS 3.1 MB 3.3 MB -135.8 KB (-4.1%) OK
bridge_cffi-stripped macOS 1.9 MB 1.7 MB +158.0 KB (+9.2%) OK
bridge_cffi Windows 3.1 MB 3.3 MB -135.0 KB (-4.2%) OK
bridge_cffi-stripped Windows 1.9 MB 1.8 MB +168.2 KB (+9.5%) OK
bridge_wasm WASM 1.8 MB 1.8 MB +56 B (+0.0%) OK

Generated by cargo size-gate · workflow run

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 24, 2026

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 15 untouched benchmarks
⏩ 91 skipped benchmarks1


Comparing feat/cffi-handle-table-wire-serialization (1c1fb80) with canary (ec9ea08)

Open in CodSpeed

Footnotes

  1. 91 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 12

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)
typescript2/app-promptfiddle/src/playground/baml-lsp-worker.ts (1)

469-487: ⚠️ Potential issue | 🟠 Major

Exception-path handle leak: partially-created BamlHandle objects are never freed

If decodeCallResult throws after constructing one or more BamlHandle instances (which are pushed into handles[]), the catch block posts the error but never frees those handles. Because they are not stored in liveHandles, the clearHandles path won't clean them up either, causing WASM memory to leak on every decoding failure.

🔒 Proposed fix: free handles in the error path
      try {
        const resultBytes = await runtime.callFunction(
          msg.id,
          msg.project,
          msg.name,
          msg.argsProto,
        );
        const bytes = new Uint8Array(resultBytes);
        const handles: BamlHandle[] = [];
        const decoded = decodeCallResult(bytes, (key, handleType, typeName) => {
          const h = new BamlHandle(key, handleType);
          handles.push(h);
          return h;
        });
        if (handles.length > 0) {
          liveHandles.set(msg.id, handles);
        }
        const result = JSON.stringify(decoded, null, 2);
        postOut({ type: "callFunctionResult", id: msg.id, result });
      } catch (e) {
+       // Free any handles created before the error to avoid WASM leaks.
+       for (const h of handles) {
+         h.free();
+       }
        postOut({
          type: "callFunctionError",
          id: msg.id,
          error: e instanceof Error ? e.message : String(e),
        });
      }

Note: handles must be declared before the try block (or the catch block must reference it via closure) for this to work:

+     const handles: BamlHandle[] = [];
      try {
        const resultBytes = await runtime.callFunction(...);
        const bytes = new Uint8Array(resultBytes);
-       const handles: BamlHandle[] = [];
        const decoded = decodeCallResult(bytes, (key, handleType, typeName) => {
          const h = new BamlHandle(key, handleType);
          handles.push(h);
          return h;
        });
        if (handles.length > 0) {
          liveHandles.set(msg.id, handles);
        }
        const result = JSON.stringify(decoded, null, 2);
        postOut({ type: "callFunctionResult", id: msg.id, result });
      } catch (e) {
+       for (const h of handles) h.free();
        postOut({
          type: "callFunctionError",
          id: msg.id,
          error: e instanceof Error ? e.message : String(e),
        });
      }
baml_language/crates/bridge_python/src/runtime.rs (1)

48-48: ⚠️ Potential issue | 🟡 Minor

Stale docstring: HostFunctionArgumentsCallFunctionArgs.

Lines 48 and 104 still reference "Protobuf-encoded HostFunctionArguments bytes" but the code now decodes CallFunctionArgs. Update to match the new proto message name.

📝 Proposed fix
-    /// * `args_proto` - Protobuf-encoded HostFunctionArguments bytes
+    /// * `args_proto` - Protobuf-encoded CallFunctionArgs bytes

Apply to both call_function (line 48) and call_function_sync (line 104).

Also applies to: 104-108

baml_language/crates/bridge_python/python_src/baml_py/proto.py (1)

84-132: 🧹 Nitpick | 🔵 Trivial

New outbound variants media_value and prompt_ast_value not handled in decoder.

The outbound proto now includes media_value (tag 17) and prompt_ast_value (tag 18) in BamlOutboundValue. These aren't decoded here — the fallback on line 132 returns None silently.

This is likely fine for in-process Python usage (for_in_process() emits handles, not inline media/prompt_ast). But if this decoder is ever used with wire-mode data, these values would silently become None.

Consider adding an explicit branch (even if it just returns a placeholder or raises) for discoverability, or document the assumption.

baml_language/crates/bridge_ctypes/types/baml/cffi/v1/baml_outbound.proto (1)

12-38: 🧹 Nitpick | 🔵 Trivial

Skipped field numbers in BamlOutboundValue (1, 10) — consider reserved directives.

Fields 1 and 10 are unused in the oneof. If these were removed from a previous schema version, adding reserved 1, 10; prevents accidental reuse and documents the gap.

baml_language/crates/bridge_ctypes/src/value_decode.rs (1)

1-138: 🧹 Nitpick | 🔵 Trivial

Add unit tests for inbound_to_external with handle resolution.

The handle table itself is well-tested, but the decoding path — particularly the handle variant (lines 38–43) and recursive handle resolution in nested structures (lines 55, 72, 93, 132) — lacks dedicated unit tests. Rust unit tests are preferred per coding guidelines; consider adding tests for edge cases like invalid handle keys and nested structures with handles.

baml_language/crates/bridge_ctypes/src/value_encode.rs (1)

22-136: 🛠️ Refactor suggestion | 🟠 Major

Missing unit tests for the new encoding logic.

This file contains non-trivial conversion logic — especially the conditional serialization paths for Media/PromptAst (lines 107-117) and the handle-table fallback (lines 119-132). Unit tests would be valuable to verify:

  1. Media/PromptAst are serialized inline when the corresponding flags are true.
  2. Media/PromptAst are inserted into the handle table when the flags are false.
  3. Round-trip consistency for primitive, collection, and nested values.

As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible" and "Always run cargo test --lib if you changed any Rust code."


ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec9ea08 and 9123ff0.

⛔ Files ignored due to path filters (1)
  • baml_language/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (44)
  • baml_language/crates/baml_lsp_server/src/playground_server.rs
  • baml_language/crates/bex_project/Cargo.toml
  • baml_language/crates/bex_project/src/lib.rs
  • baml_language/crates/bridge_cffi/src/ffi/callbacks.rs
  • baml_language/crates/bridge_cffi/src/ffi/functions.rs
  • baml_language/crates/bridge_cffi/src/ffi/handle.rs
  • baml_language/crates/bridge_cffi/src/ffi/mod.rs
  • baml_language/crates/bridge_cffi/src/ffi/objects.rs
  • baml_language/crates/bridge_cffi/src/lib.rs
  • baml_language/crates/bridge_ctypes/Cargo.toml
  • baml_language/crates/bridge_ctypes/build.rs
  • baml_language/crates/bridge_ctypes/src/error.rs
  • baml_language/crates/bridge_ctypes/src/handle_table.rs
  • baml_language/crates/bridge_ctypes/src/lib.rs
  • baml_language/crates/bridge_ctypes/src/value_decode.rs
  • baml_language/crates/bridge_ctypes/src/value_encode.rs
  • baml_language/crates/bridge_ctypes/types/baml/cffi/v1/baml_inbound.proto
  • baml_language/crates/bridge_ctypes/types/baml/cffi/v1/baml_object.proto
  • baml_language/crates/bridge_ctypes/types/baml/cffi/v1/baml_object_methods.proto
  • baml_language/crates/bridge_ctypes/types/baml/cffi/v1/baml_outbound.proto
  • baml_language/crates/bridge_python/python_src/baml/cffi/v1/baml_inbound_pb2.py
  • baml_language/crates/bridge_python/python_src/baml/cffi/v1/baml_object_pb2.py
  • baml_language/crates/bridge_python/python_src/baml/cffi/v1/baml_outbound_pb2.py
  • baml_language/crates/bridge_python/python_src/baml_py/proto.py
  • baml_language/crates/bridge_python/src/handle.rs
  • baml_language/crates/bridge_python/src/lib.rs
  • baml_language/crates/bridge_python/src/runtime.rs
  • baml_language/crates/bridge_python/src/types/collector.rs
  • baml_language/crates/bridge_wasm/src/handle.rs
  • baml_language/crates/bridge_wasm/src/lib.rs
  • typescript2/app-promptfiddle/src/playground/baml-lsp-worker.ts
  • typescript2/package.json
  • typescript2/pkg-playground/src/ExecutionPanel.tsx
  • typescript2/pkg-playground/src/ResultDisplay.tsx
  • typescript2/pkg-playground/src/ports/WebSocketRuntimePort.ts
  • typescript2/pkg-playground/src/renderers/Media.tsx
  • typescript2/pkg-playground/src/renderers/PromptAst.tsx
  • typescript2/pkg-playground/src/renderers/registerBuiltins.ts
  • typescript2/pkg-playground/src/worker-protocol.ts
  • typescript2/pkg-proto/src/decode.ts
  • typescript2/pkg-proto/src/encode.ts
  • typescript2/pkg-proto/src/index.ts
  • typescript2/pkg-proto/src/test/encode-decode.test.ts
  • typescript2/pkg-proto/src/types.ts
💤 Files with no reviewable changes (4)
  • baml_language/crates/bridge_ctypes/types/baml/cffi/v1/baml_object_methods.proto
  • baml_language/crates/bridge_ctypes/build.rs
  • baml_language/crates/bridge_ctypes/types/baml/cffi/v1/baml_object.proto
  • baml_language/crates/bridge_python/python_src/baml/cffi/v1/baml_object_pb2.py

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (2)
baml_language/crates/bridge_wasm/src/handle.rs (1)

46-53: f64 → u64 cast for NaN/negative inputs silently produces key 0.

Already flagged in a previous review — the key as u64 cast converts NaN/negative → 0, which is the invalid sentinel. Construction succeeds but subsequent operations will fail. Consider validating or documenting this in the constructor body.

baml_language/crates/bridge_ctypes/src/handle_table.rs (1)

122-146: RwLock poisoning: all .unwrap() calls will cascade-panic if the lock is ever poisoned.

Already flagged in a previous review. Since this is a global static used across all FFI bridges, a single panic while holding the lock will poison it permanently, causing every subsequent operation to panic as well. Consider using .unwrap_or_else(|e| e.into_inner()) to recover gracefully.


ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9123ff0 and df2c4a6.

📒 Files selected for processing (6)
  • baml_language/.cargo/size-gate.toml
  • baml_language/.ci/size-gate/wasm32-unknown-unknown.toml
  • baml_language/crates/bridge_cffi/src/ffi/callbacks.rs
  • baml_language/crates/bridge_ctypes/src/handle_table.rs
  • baml_language/crates/bridge_python/src/types/collector.rs
  • baml_language/crates/bridge_wasm/src/handle.rs

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
baml_language/crates/baml_builtins_macros/src/codegen_native.rs (1)

523-555: 🧹 Nitpick | 🔵 Trivial

Run cargo test --lib for this Rust change.

Please run the library tests as part of this update.
As per coding guidelines: "Always run cargo test --lib if you changed any Rust code".

baml_language/crates/bridge_ctypes/src/value_encode.rs (1)

18-137: 🧹 Nitpick | 🔵 Trivial

Add unit tests for the new handle/Media/PromptAst serialization branches.

Please add Rust unit tests covering the option-gated Media/PromptAst inline serialization and the handle-table fallback to prevent regressions in these paths. As per coding guidelines: Prefer writing Rust unit tests over integration tests where possible.

♻️ Duplicate comments (1)
baml_language/crates/bridge_ctypes/src/value_encode.rs (1)

227-273: ⚠️ Potential issue | 🟡 Minor

Preserve literal types instead of collapsing to AnyType.

Ty::Literal(_) is encoded as AnyType, which drops literal specificity for consumers. If the outbound proto supports a literal field type (e.g., BamlFieldTypeLiteral), please encode it explicitly to avoid losing schema fidelity.

#!/bin/bash
set -euo pipefail
rg -n "BamlFieldTypeLiteral|LiteralType" baml_language/crates/bridge_ctypes -g '*.proto' -g '*.rs'

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between df2c4a6 and ea97fbb.

⛔ Files ignored due to path filters (1)
  • baml_language/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • baml_language/crates/baml_builtins/src/lib.rs
  • baml_language/crates/baml_builtins_macros/src/codegen_native.rs
  • baml_language/crates/bex_vm/src/native.rs
  • baml_language/crates/bridge_ctypes/src/error.rs
  • baml_language/crates/bridge_ctypes/src/handle_table.rs
  • baml_language/crates/bridge_ctypes/src/value_encode.rs
  • baml_language/crates/bridge_python/Cargo.toml
  • baml_language/crates/bridge_python/src/types/collector.rs
  • baml_language/crates/bridge_wasm/src/handle.rs
  • typescript2/app-promptfiddle/src/playground/baml-lsp-worker.ts
  • typescript2/pkg-playground/src/ResultDisplay.tsx
  • typescript2/pkg-playground/src/ports/WebSocketRuntimePort.ts
  • typescript2/pkg-proto/src/decode.ts
  • typescript2/pkg-proto/src/test/encode-decode.test.ts

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

♻️ Duplicate comments (3)
typescript2/app-promptfiddle/src/playground/baml-lsp-worker.ts (1)

483-483: ⚠️ Potential issue | 🟡 Minor

JSON.stringify will throw on bigint values — bigint replacer still missing

decodeCallResult can return bigint values (from proto int64 fields). JSON.stringify throws a TypeError on any bigint it encounters. The fix was previously suggested but has not been applied.

🛡️ Proposed fix
-        const result = JSON.stringify(decoded, null, 2);
+        const result = JSON.stringify(
+          decoded,
+          (_, v) => (typeof v === 'bigint' ? v.toString() : v),
+          2,
+        );
baml_language/crates/bridge_ctypes/src/handle_table.rs (1)

1-3: Run Rust unit tests for this change.
Please run cargo test --lib.

As per coding guidelines: Always run cargo test --lib if you changed any Rust code.

baml_language/crates/bridge_ctypes/src/value_encode.rs (1)

227-262: ⚠️ Potential issue | 🟡 Minor

Literal type metadata is still dropped to AnyType.
If the outbound proto still supports BamlFieldTypeLiteral, consider preserving literal values instead of collapsing to AnyType.

#!/bin/bash
# Verify whether BamlFieldTypeLiteral exists in proto/generated code
rg -n "BamlFieldTypeLiteral" --type proto --type rust

# Inspect Ty::Literal to understand expected literal variants
rg -n "Ty::Literal" --type rust -C 2

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ea97fbb and a786957.

⛔ Files ignored due to path filters (1)
  • baml_language/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (11)
  • baml_language/crates/bridge_ctypes/src/error.rs
  • baml_language/crates/bridge_ctypes/src/handle_table.rs
  • baml_language/crates/bridge_ctypes/src/value_encode.rs
  • baml_language/crates/bridge_python/Cargo.toml
  • baml_language/crates/bridge_python/src/types/collector.rs
  • baml_language/crates/bridge_wasm/src/handle.rs
  • typescript2/app-promptfiddle/src/playground/baml-lsp-worker.ts
  • typescript2/pkg-playground/src/ResultDisplay.tsx
  • typescript2/pkg-playground/src/ports/WebSocketRuntimePort.ts
  • typescript2/pkg-proto/src/decode.ts
  • typescript2/pkg-proto/src/test/encode-decode.test.ts

- Proto: InboundValue reserved 1; MediaTypeEnum add MEDIA_TYPE_UNSPECIFIED
- bridge_wasm: fix BamlHandle docstrings (key, handle_type)
- baml-lsp-worker: clear liveHandles when new call returns 0 handles
- ResultDisplay: use BAML_TYPE_KEY, simplify isComplex
- WebSocketRuntimePort: emit callFunctionError on decode failure
- pkg-proto decode: exhaustive switch defaults, tryParseJson metadata,
  handle.key ?? 0, MEDIA_TYPE_UNSPECIFIED in MEDIA_TYPE_NAMES
- encode-decode.test: use BamlHandleType.FUNCTION_REF instead of magic 5

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
typescript2/pkg-proto/src/decode.ts (1)

130-221: 🛠️ Refactor suggestion | 🟠 Major

deserializeValuedefault branch silently returns null for unrecognized $case variants.

Line 218–219: when a new outbound value variant is added to the proto, the default branch returns null without logging or throwing. Unlike the media/promptAst helpers above, there's no compile-time exhaustiveness check (const _exhaustive: never) here, so new proto variants will silently disappear.

Consider adding an exhaustive check (same pattern as the other deserializers) to get compile-time breakage when new variants are introduced.

Proposed fix
-    default:
-      return null;
+    default: {
+      const _exhaustive: never = holder.value;
+      return null;
+    }
typescript2/pkg-proto/src/test/encode-decode.test.ts (1)

95-257: 🛠️ Refactor suggestion | 🟠 Major

No test coverage for mediaValue and promptAstValue deserialization.

decode.ts introduces substantial new logic in deserializeMedia, deserializePromptAstSimple, and deserializePromptAst, but these code paths have no corresponding tests. Consider adding tests that:

  1. Encode a mediaValue with url/base64/file variants and verify the decoded BamlJsMedia shape.
  2. Encode a promptAstValue with simple/message/multiple variants (including nested structures and metadata) and verify the decoded BamlJsPromptAst shape.
  3. Verify that the tryParseJson fallback works when metadataAsJson is invalid JSON.

These are the most complex new deserializers in this PR and would benefit from dedicated encode-decode round-trip tests.

typescript2/app-promptfiddle/src/playground/baml-lsp-worker.ts (2)

487-493: ⚠️ Potential issue | 🟡 Minor

Handles created before decodeCallResult throws are leaked on the error path.

If decodeCallResult (or JSON.stringify) throws after the callback has already created some BamlHandle instances, those handles sit in the local handles array but are never freed — they don't reach liveHandles and .free() is never called.

🛡️ Proposed fix: free partially-created handles in catch
       } catch (e) {
+        for (const h of handles) h.free();
         postOut({
           type: "callFunctionError",
           id: msg.id,
           error: e instanceof Error ? e.message : String(e),
         });
       }

Note: handles needs to be declared outside the try block for this to work. Move the declaration to before the try:

+      const handles: BamlHandle[] = [];
       try {
         const resultBytes = await runtime.callFunction(
           msg.id,
           msg.project,
           msg.name,
           msg.argsProto,
         );
         const bytes = new Uint8Array(resultBytes);
-        const handles: BamlHandle[] = [];
         const decoded = decodeCallResult(bytes, (key, handleType, typeName) => {

54-70: ⚠️ Potential issue | 🟠 Major

dispose() does not free handles in liveHandles, leaking WASM memory.

When the worker is disposed, all BamlHandle objects stored in liveHandles should be explicitly freed — same pattern as clearHandles. Without this, outstanding WASM allocations are never reclaimed deterministically.

🔒 Proposed fix: free all live handles on dispose
 function dispose(): void {
   if (disposed) return;
   disposed = true;
+  // Free all outstanding WASM handles
+  for (const handles of liveHandles.values()) {
+    for (const h of handles) h.free();
+  }
+  liveHandles.clear();
   // Resolve any pending env requests so awaiting callers don't hang
   for (const resolve of pendingEnvResolvers.values()) {
♻️ Duplicate comments (4)
typescript2/pkg-proto/src/decode.ts (2)

56-75: Exhaustive switch and null guard addressed from prior feedback.

The default branch with const _exhaustive: never = m.value at Line 71 ensures compile-time safety if the proto oneof gains new variants, and the runtime fallback is reasonable.


202-210: Handle key undefined guard addressed from prior feedback.

Line 205's BigInt(handle.key ?? 0) correctly defaults an unset key to 0n, preventing a TypeError from BigInt(undefined).

typescript2/pkg-proto/src/test/encode-decode.test.ts (1)

240-257: Handle wrapping test uses symbolic enum constant — past feedback addressed.

Uses BamlHandleType.FUNCTION_REF instead of a magic number, and properly asserts all three callback arguments (key, handleType, typeName) plus the final decoded shape. Good coverage of this path.

baml_language/crates/bridge_ctypes/src/handle_table.rs (1)

1-2: Reminder: run cargo test --lib for this Rust change.

As per coding guidelines: Always run cargo test --lib if you changed any Rust code.


ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a786957 and e27fb35.

📒 Files selected for processing (9)
  • baml_language/crates/bridge_ctypes/src/handle_table.rs
  • baml_language/crates/bridge_ctypes/types/baml/cffi/v1/baml_inbound.proto
  • baml_language/crates/bridge_ctypes/types/baml/cffi/v1/baml_outbound.proto
  • baml_language/crates/bridge_wasm/src/handle.rs
  • typescript2/app-promptfiddle/src/playground/baml-lsp-worker.ts
  • typescript2/pkg-playground/src/ResultDisplay.tsx
  • typescript2/pkg-playground/src/ports/WebSocketRuntimePort.ts
  • typescript2/pkg-proto/src/decode.ts
  • typescript2/pkg-proto/src/test/encode-decode.test.ts

- Use arc_prompt_ast_to_proto / arc_prompt_ast_simple_to_proto so .map(fn)
  satisfies redundant-closure review (lines 188, 217)
- Map Ty::Literal to FieldType::LiteralType(BamlFieldTypeLiteral) instead of
  AnyType; add literal_to_field_type_literal and baml_type dep

Co-authored-by: Cursor <cursoragent@cursor.com>
Merged via the queue into canary with commit 7c71c3e Feb 25, 2026
46 checks passed
@hellovai hellovai deleted the feat/cffi-handle-table-wire-serialization branch February 25, 2026 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant