Skip to content

Sam/call fn - #3592

Closed
sxlijin wants to merge 5 commits into
canaryfrom
sam/call-fn
Closed

sxlijin wants to merge 5 commits into
canaryfrom
sam/call-fn

Conversation

@sxlijin

@sxlijin sxlijin commented May 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features

    • Built-in native functions and system operations can now be invoked directly as entry points.
  • Bug Fixes

    • External operation failures and unhandled throws now include captured VM stack traces for clearer error reports.
    • Sys-op lifecycle handling improved to surface errors and results consistently.
  • SDK / Runtimes

    • Bridge and language runtimes (C-FFI, Node, Python) now perform the final protobuf encoding at the language boundary for consistent outbound payloads.

Review Change Stack

@vercel

vercel Bot commented May 28, 2026 •

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
beps Ready Ready Preview, Comment May 29, 2026 12:46am
promptfiddle Building Building Preview, Comment May 29, 2026 12:46am
promptfiddle2 Error Error May 29, 2026 12:46am

Request Review

@coderabbitai

coderabbitai Bot commented May 28, 2026 •

Copy link
Copy Markdown
Contributor

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

Refactors VM native-call model into separate call and continuation frames, converts sys-op execution to a frame-based request/completion protocol, updates engine host-call and error routing to capture VM stack traces, and moves final protobuf encoding from the C-FFI bridge into SDK boundaries.

Changes

Native entry, VM frame model, sys-op protocol, error handling, and bridge encoding

Layer / File(s) Summary
VM native frames, CPS, and GC/root updates
baml_language/crates/bex_vm/src/vm.rs
Introduce NativeCallFrame and NativeContinuationFrame, remove pending_call_type_args, update GC root collection/forwarding, and rework exec_compact to run native call/continuation frames and handle yields.
Entry-point seeding and call-type-arg plumbing
baml_language/crates/bex_vm/src/vm.rs, baml_language/crates/bex_engine/src/lib.rs
set_entry_point_with_type_args now seeds bytecode or native-call frames; sys-op entry points use set_sysop_entry_point_with_type_args. Call-site type-args are threaded into execute_call_from_locals_offset and stored on native frames.
Sys-op request/completion and VM APIs
baml_language/crates/bex_vm/src/vm.rs
VmExecState::SysOp becomes unit; OpCode::SysOp pushes Frame::SysOpCall; add SysOpRequest<'a>, SysOpCompletion, and BexVm APIs active_sysop_request, complete_sysop, throw_from_active_sysop, capture_stack_trace_for_active_error.
Exception unwinding and stack-trace capture
baml_language/crates/bex_vm/src/vm.rs, baml_language/crates/bex_vm/src/errors.rs
Treat NativeCall/NativeContinuation/SysOpCall as having no exception handlers; draining eval-stack only on NativeCall pops; native/sys-op frames in traces show error_line: 0. New VmInternalError variants for missing/reentered sys-op state added.
Engine sys-op execution loop and host-call wiring
baml_language/crates/bex_engine/src/lib.rs
Engine VmExecState::SysOp arm now reads the active sys-op request from the VM, converts vm_args→bex_args, captures return-type from vm_args, and completes sys-ops via thread.vm.complete_sysop(...) branching on SysOpCompletion to resume or return a coerced root value.
Engine error handling and host-callable throw routing
baml_language/crates/bex_engine/src/lib.rs
Add EngineError::ExternalOpFailedWithTrace { source, trace } and format_external_op_failed_with_trace. Refactor deliver_host_call_throw to Result<Option<ThreadOutcome>, EngineError> to route catchable throws via active sys-op resume/complete/unhandled paths; non-catchable op errors capture VM traces into ExternalOpFailedWithTrace.
Bridge encoding responsibility moved to SDK boundaries
baml_language/crates/bridge_cffi/*, baml_language/sdks/nodejs/bridge_nodejs/src/runtime.rs, baml_language/sdks/python/rust/bridge_python/src/runtime.rs
call_and_encode now returns BamlOutboundResult; C-FFI lib encodes via .encode_to_vec() before sending and maps encode-stage panics to outbound panic envelopes. Node and Python runtimes now call .encode_to_vec() on the call_and_encode result before returning bytes.
baml.json native helpers
baml_language/crates/bex_vm/src/package_baml/json.rs
Add first_native_type_arg helper reading type-args from the active NativeCall frame; rewire to_string/from_string/from_json to use it and centralize missing-type-arg errors.
Tests and small docs/runner updates
baml_language/crates/bex_engine/tests/function_resolution.rs, baml_language/crates/bex_vm/src/lib.rs, baml_language/crates/bex_vm_types/src/bytecode.rs, baml_language/crates/tools_onionskin/src/compiler.rs
Replace previous native-entry rejection test with tests that assert native (baml.math.trunc) and sysop (baml.fs.exists) entry invocation succeed; reformat re-export list; reword Instruction::SysOp docs and adjust VM Runner sys-op match pattern.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

  • BoundaryML/baml#3342: Overlaps on VM error taxonomy and native-function error handling changes.
  • BoundaryML/baml#3076: Related to sys-op lifecycle and snapshot-removal refactor introducing VM sys-op APIs.

"I'm a rabbit in the VM wood, I hopped through frames and understood,
Sys-ops now knock and wait their turn, native calls yield then duly return,
Traces stitched when ops misbehave, bridges hand off the final save,
Tiny hops of code unite — hop on, reviewers, for a careful sight." 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The pull request title 'Sam/call fn' is vague and generic, using non-descriptive terms that do not meaningfully convey the actual changeset, which involves significant refactoring of native function and sys-op handling across the VM and engine. Replace the title with a more descriptive summary of the main change, such as 'Refactor native function and sys-op execution to use explicit call frames' or 'Support entry-point invocation for native and sys-op functions'.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sam/call-fn

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0fdeef7514

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

let bytes = match encoded {
Ok(bytes) => bytes,
let bytes = match outbound {
Ok(result) => result.encode_to_vec(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep encoding inside the C-ABI panic guard

When BamlOutboundResult::encode_to_vec() panics in this C-ABI async path, the panic is now outside the AssertUnwindSafe(...).catch_unwind() scope because call_and_encode returns the unencoded result; the spawned task will panic before send_outbound_result_to_callback, so the host never receives the promised SdkPanic envelope. This regresses the safety net described in this block for encode-stage panics; wrap the encoding itself in the catch boundary or move it back into the guarded future.

Useful? React with 👍 / 👎.

@blacksmith-sh

This comment has been minimized.

@vercel
vercel Bot temporarily deployed to Preview – beps May 28, 2026 19:59 Inactive
@github-actions

github-actions Bot commented May 28, 2026 •

Copy link
Copy Markdown

Binary size checks passed

✅ 7 passed

Artifact Platform Gzip Baseline Delta Status
✅ bridge_cffi Linux 6.6 MB 6.6 MB +33.2 KB (+0.5%) OK
✅ bridge_cffi-stripped Linux 5.7 MB 5.7 MB -19.9 KB (-0.3%) OK
✅ bridge_cffi macOS 5.5 MB 5.1 MB +371.7 KB (+7.3%) OK
✅ bridge_cffi-stripped macOS 4.7 MB 4.7 MB -4.1 KB (-0.1%) OK
✅ bridge_cffi Windows 5.5 MB 5.1 MB +379.9 KB (+7.5%) OK
✅ bridge_cffi-stripped Windows 4.8 MB 4.7 MB +113.6 KB (+2.4%) OK
✅ bridge_wasm WASM 3.7 MB 3.8 MB -75.5 KB (-2.0%) OK

Generated by cargo size-gate · workflow run

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

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

⚠️ Outside diff range comments (1)
baml_language/crates/bridge_cffi/src/lib.rs (1)

190-201: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

catch_unwind no longer protects the encoding step.

On Line 199-Line 201, .encode_to_vec() runs after the unwind boundary from Line 190-Line 197. If encoding panics, this path can exit before send_outbound_result_to_callback, so the caller may not receive an envelope response.

💡 Proposed fix
-        let outbound = AssertUnwindSafe(call_and_encode(
-            runtime,
-            func_name,
-            kwargs.into(),
-            call_ctx.build(),
-        ))
-        .catch_unwind()
-        .await;
-
-        let bytes = match outbound {
-            Ok(result) => result.encode_to_vec(),
+        let bytes = match AssertUnwindSafe(async {
+            call_and_encode(runtime, func_name, kwargs.into(), call_ctx.build())
+                .await
+                .encode_to_vec()
+        })
+        .catch_unwind()
+        .await
+        {
+            Ok(bytes) => bytes,
             // An encode-stage panic also rides the envelope as an `SdkPanic`,
             // uniform with every other result.
             Err(panic_info) => baml_to_host::panic_to_outbound(panic_info.as_ref()),
         };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@baml_language/crates/bridge_cffi/src/lib.rs` around lines 190 - 201, The
encoding step (.encode_to_vec()) currently runs outside the
AssertUnwindSafe(...).catch_unwind() boundary so an encode-time panic can bypass
send_outbound_result_to_callback; move the encode work into the protected unwind
boundary by ensuring call_and_encode (or the closure passed to AssertUnwindSafe)
returns the already-encoded Vec<u8> (or call encode_to_vec() inside that
AssertUnwindSafe closure) so that the match on outbound handles both successful
encoded bytes and panics and always routes through
send_outbound_result_to_callback; update references to call_and_encode,
AssertUnwindSafe, outbound, encode_to_vec, and send_outbound_result_to_callback
accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@baml_language/crates/bex_vm/src/vm.rs`:
- Around line 125-129: The NativeContinuationFrame loses the suspended native
call’s type_args, so calls to current_call_type_args() after a YieldToCall
return outer/pending generics; add a type_args field to NativeContinuationFrame
(same type as in NativeCallFrame), populate it when you create/replace a
NativeCallFrame with a NativeContinuationFrame (the YieldToCall path where you
pop NativeCallFrame), and update current_call_type_args() so it also inspects
NativeContinuationFrame for type_args. Apply the same change to the other
similar locations that construct NativeContinuationFrame (the other spots
referenced) so suspended native frames always preserve their type arguments.
- Around line 2581-2589: The native-call branch currently seeds type_args with
only pending_call_type_args which drops implicit generics; update the
FunctionKind::Native(_) handling to mirror the bytecode branch by combining
pending_call_type_args with closure_type_args and bound_method_class_type_args
when constructing the type_args for the NativeCallFrame (the same set the
bytecode path uses), then push Frame::NativeCall(NativeCallFrame { function:
callee_ptr, locals_offset, arg_count, type_args }) and set *frame_idx
accordingly so generic native closures and bound methods retain their implicit
frame generics.

---

Outside diff comments:
In `@baml_language/crates/bridge_cffi/src/lib.rs`:
- Around line 190-201: The encoding step (.encode_to_vec()) currently runs
outside the AssertUnwindSafe(...).catch_unwind() boundary so an encode-time
panic can bypass send_outbound_result_to_callback; move the encode work into the
protected unwind boundary by ensuring call_and_encode (or the closure passed to
AssertUnwindSafe) returns the already-encoded Vec<u8> (or call encode_to_vec()
inside that AssertUnwindSafe closure) so that the match on outbound handles both
successful encoded bytes and panics and always routes through
send_outbound_result_to_callback; update references to call_and_encode,
AssertUnwindSafe, outbound, encode_to_vec, and send_outbound_result_to_callback
accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b8346eed-3400-48cb-b4e7-f6a921e8d115

📥 Commits

Reviewing files that changed from the base of the PR and between 3500293 and 0fdeef7.

📒 Files selected for processing (10)
  • baml_language/crates/bex_engine/src/lib.rs
  • baml_language/crates/bex_engine/tests/builtin_entry_points.rs
  • baml_language/crates/bex_engine/tests/function_resolution.rs
  • baml_language/crates/bex_vm/src/errors.rs
  • baml_language/crates/bex_vm/src/vm.rs
  • baml_language/crates/bridge_cffi/src/baml_to_host.rs
  • baml_language/crates/bridge_cffi/src/ffi/callbacks.rs
  • baml_language/crates/bridge_cffi/src/lib.rs
  • baml_language/sdks/nodejs/bridge_nodejs/src/runtime.rs
  • baml_language/sdks/python/rust/bridge_python/src/runtime.rs

Comment thread baml_language/crates/bex_vm/src/vm.rs Resolved
Comment thread baml_language/crates/bex_vm/src/vm.rs Resolved
@vercel
vercel Bot temporarily deployed to Preview – promptfiddle2 May 28, 2026 20:17 Inactive
@vercel
vercel Bot temporarily deployed to Preview – promptfiddle May 28, 2026 20:17 Inactive
@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Actionable comments posted: 0

@blacksmith-sh

This comment has been minimized.

@vercel
vercel Bot temporarily deployed to Preview – beps May 28, 2026 20:23 Inactive

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
baml_language/crates/bex_vm/src/vm.rs (2)

125-130: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve native type_args across YieldToCall.

current_call_type_args() stops working after a native callback yield because the suspended frame is converted into NativeContinuationFrame without carrying its type_args, and the accessor only checks Frame::NativeCall. Any resumed continuation that asks for the active generics now gets [].

Suggested fix
 pub struct NativeContinuationFrame {
     pub(crate) function: HeapPtr,
     pub(crate) continuation: Option<Box<dyn crate::package_baml::Continuation>>,
+    pub(crate) type_args: Vec<baml_type::Ty>,
 }

 pub fn current_call_type_args(&self) -> &[baml_type::Ty] {
-    if let Some(Frame::NativeCall(frame)) = self.frames.last() {
-        return &frame.type_args;
+    match self.frames.last() {
+        Some(Frame::NativeCall(frame)) => return &frame.type_args,
+        Some(Frame::NativeContinuation(frame)) => return &frame.type_args,
+        _ => {}
     }
     &[]
 }

-                        let frame = self.frames.pop().expect("native frame stack is not empty");
-                        match frame {
-                            Frame::NativeCall(frame) => {
-                                self.stack.truncate(frame.locals_offset.raw());
-                            }
-                            Frame::NativeContinuation(_) => {}
+                        let type_args = match self.frames.pop().expect("native frame stack is not empty") {
+                            Frame::NativeCall(frame) => {
+                                self.stack.truncate(frame.locals_offset.raw());
+                                frame.type_args
+                            }
+                            Frame::NativeContinuation(frame) => frame.type_args,
                             Frame::Bytecode(_) | Frame::SysOpCall(_) => {
                                 unreachable!("native yield handler requires native frame")
                             }
-                        }
+                        };
                         self.frames
                             .push(Frame::NativeContinuation(NativeContinuationFrame {
                                 function: native_fn_ptr,
                                 continuation: Some(continuation),
+                                type_args,
                             }));

Also applies to: 977-986, 3558-3572

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@baml_language/crates/bex_vm/src/vm.rs` around lines 125 - 130, The suspended
native callback frame loses its generics because NativeContinuationFrame lacks a
type_args field and current_call_type_args only inspects Frame::NativeCall; add
a type_args: Vec<TypeArg> (or the existing TypeArgs type) to
NativeContinuationFrame, populate it wherever a frame is converted into a
NativeContinuationFrame (e.g. in YieldToCall / the suspension code paths), and
update current_call_type_args to also return that field for
Frame::NativeContinuationFrame; make corresponding changes to any constructor or
resume logic that builds or restores NativeContinuationFrame so the type_args
are preserved across yields.

2719-2759: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Seed native frames with implicit closure / bound-method generics too.

The native branch still stores only call_type_args. That drops closure_type_args and bound_method_class_type_args, so current_call_type_args() inside generic native closures and native methods diverges from the bytecode path.

Suggested fix
         match callee.kind {
             FunctionKind::Native(_) => {
+                let mut type_args = if !bound_method_class_type_args.is_empty() {
+                    bound_method_class_type_args
+                } else {
+                    closure_type_args
+                };
+                type_args.extend(call_type_args);
                 self.frames.push(Frame::NativeCall(NativeCallFrame {
                     function: callee_ptr,
                     locals_offset,
                     arg_count,
-                    type_args: call_type_args,
+                    type_args,
                 }));
                 *frame_idx = self.frames.len() - 1;
             }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@baml_language/crates/bex_vm/src/vm.rs` around lines 2719 - 2759, The native
call branch in FunctionKind::Native only stores call_type_args, dropping
closure_type_args and bound_method_class_type_args so generics diverge from the
bytecode path; construct initial_type_args the same way as in the Bytecode
branch (use bound_method_class_type_args unless empty else closure_type_args,
then extend with call_type_args) and pass that initial_type_args into the
NativeCallFrame.type_args when pushing the frame (symbols: FunctionKind::Native,
NativeCallFrame { type_args }, closure_type_args, bound_method_class_type_args,
call_type_args, current_call_type_args()).
🧹 Nitpick comments (1)
baml_language/crates/bex_engine/src/lib.rs (1)

1918-1918: 💤 Low value

Unnecessary .clone() on throw_value.

throw_value is already owned from the if let Some(throw_value) binding and is not used after this line. convert_external_to_vm_value takes BexExternalValue by value, so the clone is unnecessary.

♻️ Suggested fix
-        let value = self.convert_external_to_vm_value(thread, throw_value.clone())?;
+        let value = self.convert_external_to_vm_value(thread, throw_value)?;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@baml_language/crates/bex_engine/src/lib.rs` at line 1918, The call
unnecessarily clones throw_value; in the match arm where you have if let
Some(throw_value) the binding owns the BexExternalValue and
convert_external_to_vm_value(thread, BexExternalValue) takes it by value, so
remove the .clone() and pass throw_value directly to
convert_external_to_vm_value to avoid an unnecessary allocation (refer to
convert_external_to_vm_value and the throw_value binding in lib.rs).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 1276-1282: Update the docstring for the
EngineError::NotInvokableAsEntry variant to match the runtime check in call
sites like the code that calls self.lookup_function and rejects only
bex_vm_types::FunctionKind::NativeUnresolved; the current docstring ("Only
bytecode functions can be called via `call_function`") is stale—change it to
state that SysOp and resolved Native functions are allowed and that only
NativeUnresolved functions (or unresolved/native entry points) cannot be invoked
as an entry, referencing the variant name EngineError::NotInvokableAsEntry and
the lookup/validation logic that checks function_kind.

---

Duplicate comments:
In `@baml_language/crates/bex_vm/src/vm.rs`:
- Around line 125-130: The suspended native callback frame loses its generics
because NativeContinuationFrame lacks a type_args field and
current_call_type_args only inspects Frame::NativeCall; add a type_args:
Vec<TypeArg> (or the existing TypeArgs type) to NativeContinuationFrame,
populate it wherever a frame is converted into a NativeContinuationFrame (e.g.
in YieldToCall / the suspension code paths), and update current_call_type_args
to also return that field for Frame::NativeContinuationFrame; make corresponding
changes to any constructor or resume logic that builds or restores
NativeContinuationFrame so the type_args are preserved across yields.
- Around line 2719-2759: The native call branch in FunctionKind::Native only
stores call_type_args, dropping closure_type_args and
bound_method_class_type_args so generics diverge from the bytecode path;
construct initial_type_args the same way as in the Bytecode branch (use
bound_method_class_type_args unless empty else closure_type_args, then extend
with call_type_args) and pass that initial_type_args into the
NativeCallFrame.type_args when pushing the frame (symbols: FunctionKind::Native,
NativeCallFrame { type_args }, closure_type_args, bound_method_class_type_args,
call_type_args, current_call_type_args()).

---

Nitpick comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Line 1918: The call unnecessarily clones throw_value; in the match arm where
you have if let Some(throw_value) the binding owns the BexExternalValue and
convert_external_to_vm_value(thread, BexExternalValue) takes it by value, so
remove the .clone() and pass throw_value directly to
convert_external_to_vm_value to avoid an unnecessary allocation (refer to
convert_external_to_vm_value and the throw_value binding in lib.rs).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8e0f2190-ad4e-440b-a9c3-282e0e1b28cd

📥 Commits

Reviewing files that changed from the base of the PR and between 8945bfc and 7e06b6f.

📒 Files selected for processing (8)
  • baml_language/crates/bex_engine/src/lib.rs
  • baml_language/crates/bex_engine/tests/builtin_entry_points.rs
  • baml_language/crates/bex_vm/src/errors.rs
  • baml_language/crates/bex_vm/src/lib.rs
  • baml_language/crates/bex_vm/src/package_baml/json.rs
  • baml_language/crates/bex_vm/src/vm.rs
  • baml_language/crates/bex_vm_types/src/bytecode.rs
  • baml_language/crates/tools_onionskin/src/compiler.rs
✅ Files skipped from review due to trivial changes (3)
  • baml_language/crates/bex_vm/src/lib.rs
  • baml_language/crates/bex_vm_types/src/bytecode.rs
  • baml_language/crates/bex_vm/src/package_baml/json.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • baml_language/crates/bex_engine/tests/builtin_entry_points.rs

@blacksmith-sh

This comment has been minimized.

@vercel
vercel Bot temporarily deployed to Preview – promptfiddle2 May 28, 2026 20:40 Inactive
@vercel
vercel Bot temporarily deployed to Preview – promptfiddle May 28, 2026 20:41 Inactive
@vercel
vercel Bot temporarily deployed to Preview – beps May 28, 2026 21:37 Inactive

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@baml_language/crates/bex_engine/tests/function_resolution.rs`:
- Around line 145-146: The test currently asserts filesystem sysop behavior
against the hardcoded root path "/" which is host-dependent and flaky; update
the test to use a guaranteed-existing path such as std::env::temp_dir() (or
std::env::current_dir()) instead of "/" in the sysop assertion and any related
asserts in the same test block (the tokio test annotated with #[tokio::test]
that checks filesystem sysop behavior around lines 145–161); convert the PathBuf
to a string in a platform-safe way (e.g., using to_string_lossy or to_str with
fallback) before passing it to the sysop call so the test is reliable across
CI/containers.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 68970235-3336-4829-9ed2-64e7b36f07a4

📥 Commits

Reviewing files that changed from the base of the PR and between 7e06b6f and e844a04.

📒 Files selected for processing (13)
  • baml_language/crates/bex_engine/src/lib.rs
  • baml_language/crates/bex_engine/tests/function_resolution.rs
  • baml_language/crates/bex_vm/src/errors.rs
  • baml_language/crates/bex_vm/src/lib.rs
  • baml_language/crates/bex_vm/src/package_baml/json.rs
  • baml_language/crates/bex_vm/src/vm.rs
  • baml_language/crates/bex_vm_types/src/bytecode.rs
  • baml_language/crates/bridge_cffi/src/baml_to_host.rs
  • baml_language/crates/bridge_cffi/src/ffi/callbacks.rs
  • baml_language/crates/bridge_cffi/src/lib.rs
  • baml_language/crates/tools_onionskin/src/compiler.rs
  • baml_language/sdks/nodejs/bridge_nodejs/src/runtime.rs
  • baml_language/sdks/python/rust/bridge_python/src/runtime.rs
✅ Files skipped from review due to trivial changes (4)
  • baml_language/crates/bex_vm/src/lib.rs
  • baml_language/crates/bridge_cffi/src/ffi/callbacks.rs
  • baml_language/crates/tools_onionskin/src/compiler.rs
  • baml_language/crates/bex_vm_types/src/bytecode.rs
🚧 Files skipped from review as they are similar to previous changes (8)
  • baml_language/sdks/python/rust/bridge_python/src/runtime.rs
  • baml_language/sdks/nodejs/bridge_nodejs/src/runtime.rs
  • baml_language/crates/bex_vm/src/package_baml/json.rs
  • baml_language/crates/bridge_cffi/src/baml_to_host.rs
  • baml_language/crates/bridge_cffi/src/lib.rs
  • baml_language/crates/bex_vm/src/errors.rs
  • baml_language/crates/bex_engine/src/lib.rs
  • baml_language/crates/bex_vm/src/vm.rs

Comment on lines +145 to +146
/// filesystem sysop and return a bool. `/` exists on the test host.
#[tokio::test]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid host-dependent root-path assertions in sysop test.

"/" can be environment-dependent in CI/containers; this can make the test flaky. Use a guaranteed-existing path from std::env::temp_dir() (or current dir) instead of hardcoding root.

Suggested fix
 #[tokio::test]
 async fn sysop_fs_exists_callable_as_entry_point() {
     let eng = engine(&[("main.baml", "function main() -> int { 1 }")]);
+    let existing_path = std::env::temp_dir().to_string_lossy().to_string();

     let result = eng
         .call_function(
             "baml.fs.exists",
-            vec![BexExternalValue::String("/".to_string())],
+            vec![BexExternalValue::String(existing_path)],
             FunctionCallContextBuilder::new(CallId::next()).build(),
             true,
         )
         .await;

     match result {
-        Ok(BexExternalValue::Bool(exists)) => assert!(exists, "`/` should exist"),
+        Ok(BexExternalValue::Bool(exists)) => assert!(exists, "temp_dir should exist"),
         other => panic!("expected Ok(Bool(true)) from baml.fs.exists as entry, got {other:?}"),
     }
 }

Also applies to: 153-161

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@baml_language/crates/bex_engine/tests/function_resolution.rs` around lines
145 - 146, The test currently asserts filesystem sysop behavior against the
hardcoded root path "/" which is host-dependent and flaky; update the test to
use a guaranteed-existing path such as std::env::temp_dir() (or
std::env::current_dir()) instead of "/" in the sysop assertion and any related
asserts in the same test block (the tokio test annotated with #[tokio::test]
that checks filesystem sysop behavior around lines 145–161); convert the PathBuf
to a string in a platform-safe way (e.g., using to_string_lossy or to_str with
fallback) before passing it to the sysop call so the test is reliable across
CI/containers.

@vercel
vercel Bot temporarily deployed to Preview – beps May 28, 2026 21:55 Inactive

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
baml_language/crates/bex_vm/src/package_baml/json.rs (1)

81-94: 💤 Low value

Misleading diagnostic from reusing MissingNativeFunction.

MissingNativeFunction formats as missing native function: {name}, so the "no active native call frame" branch yields the doubled-up, confusing string missing native function: baml.json.from_string: missing active native call frame. This PR already adds purpose-built variants (ExpectedActiveSysOp) for the analogous sys-op case; consider a similarly named native-call variant (or at least drop the misleading prefix) so these fatal internal errors read clearly when debugging.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@baml_language/crates/bex_vm/src/package_baml/json.rs` around lines 81 - 94,
The error message produced by first_native_type_arg currently reuses
VmInternalError::MissingNativeFunction which yields a confusing prefix; update
the error to use a new, purpose-built variant (e.g.,
VmInternalError::ExpectedActiveNativeCall or similar) and return that from
first_native_type_arg instead of MissingNativeFunction, adjusting the
construction of the VmRustFnError accordingly; add the new enum variant to
VmInternalError with a clear name and message (or at minimum remove the
misleading "missing native function" prefix in the message), and update any
pattern matches/usages of VmInternalError to handle the new variant (refer to
first_native_type_arg, VmRustFnError, and VmInternalError).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@baml_language/crates/bex_vm/src/package_baml/json.rs`:
- Around line 81-94: The error message produced by first_native_type_arg
currently reuses VmInternalError::MissingNativeFunction which yields a confusing
prefix; update the error to use a new, purpose-built variant (e.g.,
VmInternalError::ExpectedActiveNativeCall or similar) and return that from
first_native_type_arg instead of MissingNativeFunction, adjusting the
construction of the VmRustFnError accordingly; add the new enum variant to
VmInternalError with a clear name and message (or at minimum remove the
misleading "missing native function" prefix in the message), and update any
pattern matches/usages of VmInternalError to handle the new variant (refer to
first_native_type_arg, VmRustFnError, and VmInternalError).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7e39e1d4-6135-4329-bbaa-beb511e69d47

📥 Commits

Reviewing files that changed from the base of the PR and between e844a04 and 31774a2.

📒 Files selected for processing (7)
  • baml_language/crates/bex_engine/src/lib.rs
  • baml_language/crates/bex_vm/src/errors.rs
  • baml_language/crates/bex_vm/src/lib.rs
  • baml_language/crates/bex_vm/src/package_baml/json.rs
  • baml_language/crates/bex_vm/src/vm.rs
  • baml_language/crates/bex_vm_types/src/bytecode.rs
  • baml_language/crates/tools_onionskin/src/compiler.rs
✅ Files skipped from review due to trivial changes (2)
  • baml_language/crates/bex_vm/src/lib.rs
  • baml_language/crates/bex_vm_types/src/bytecode.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • baml_language/crates/bex_engine/src/lib.rs
  • baml_language/crates/tools_onionskin/src/compiler.rs
  • baml_language/crates/bex_vm/src/vm.rs

@blacksmith-sh

This comment has been minimized.

@vercel
vercel Bot temporarily deployed to Preview – promptfiddle2 May 28, 2026 22:13 Inactive
@vercel
vercel Bot temporarily deployed to Preview – promptfiddle May 28, 2026 22:13 Inactive
@vercel
vercel Bot temporarily deployed to Preview – beps May 29, 2026 00:29 Inactive
@vercel
vercel Bot temporarily deployed to Preview – promptfiddle2 May 29, 2026 00:46 Inactive
@vercel
vercel Bot temporarily deployed to Preview – promptfiddle May 29, 2026 00:47 Inactive
@sxlijin sxlijin closed this May 30, 2026
@sxlijin
sxlijin deleted the sam/call-fn branch May 30, 2026 03:21

This branch was previously deployed

3 inactive deployments
Preview – promptfiddle — 9c1799d9 Deployed May 29, 2026 by vercel[bot]
Preview – promptfiddle2 — 9c1799d9 Deployed May 29, 2026 by vercel[bot]
Preview – beps — 9c1799d9 Deployed May 29, 2026 by vercel[bot]
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