Host values in BAML#3571
Merged
Merged
Conversation
It's an Arc containing a key and some metadata. It represents some external object from the host. When dropped, it notifies the host that it can free the object.
- `HOST_VALUE_CALLABLE` handle type (unlike other handles, this is issued from host->BAML. However it can be passed either direction) - Host callable error messages
Represents a function-typed host object in the VM. In the type system it looks like just a normal function.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds host-callable values end-to-end: new HostValue/HostClosure types, VM CallIndirect dispatch to a host sys-op, engine conversions and return-type validation, structured HostCallable errors, updated serializers/protos, and C/WASM/Node/Python/Go bridges with extensive tests. ChangesEnd-to-end host-callable plumbing
Sequence Diagram(s)sequenceDiagram
rect rgba(200, 230, 255, 0.5)
participant Client as VM
participant Engine
participant SysOps
end
rect rgba(220, 255, 220, 0.5)
participant Bridge as C/WASM/Node/Py/Go
participant Host
end
Client->>Engine: CallIndirect HostClosure(args, ret_ty)
Engine->>SysOps: io.host.call_host_value(handle,args,ret_ty)
SysOps->>Bridge: dispatch(key, callId, argsBytes)
Bridge->>Host: invoke callable(callId, args)
Host-->>Bridge: complete(callId, result | HostCallableError)
Bridge-->>SysOps: completeHostCall(callId, isError, content)
SysOps-->>Engine: value or OpError(HostCallable)
Engine-->>Client: value or throw baml.errors.HostCallable
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
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_wasm/src/handle.rs (1)
108-111:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSkip
HANDLE_TABLE.releaseforHOST_VALUE_CALLABLEinDrop.Line 110 currently releases every key through
HANDLE_TABLE, butHOST_VALUE_CALLABLEkeys belong to the host-value registry. This can accidentally release unrelated HANDLE_TABLE entries if keys overlap.🔧 Proposed fix
impl Drop for BamlHandle { fn drop(&mut self) { - HANDLE_TABLE.release(self.key); + match self.handle_type { + BamlHandleType::HostValueCallable => { + // HostValue lifetime is released via host_release_dispatch when + // the engine drops the last HostValueArc clone. + } + _ => HANDLE_TABLE.release(self.key), + } } }🤖 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_wasm/src/handle.rs` around lines 108 - 111, The Drop impl for BamlHandle currently always calls HANDLE_TABLE.release(self.key) which incorrectly releases keys that belong to the host-value registry; change the Drop implementation to first check the handle kind/variant (the BamlHandle field that indicates HOST_VALUE_CALLABLE) and skip calling HANDLE_TABLE.release when the handle is HOST_VALUE_CALLABLE (i.e., return early or match on the variant and only call HANDLE_TABLE.release for non-HOST_VALUE_CALLABLE cases), referencing the BamlHandle Drop impl, the HOST_VALUE_CALLABLE constant/variant, and HANDLE_TABLE.release to locate the code to change.
🧹 Nitpick comments (1)
baml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.ts (1)
214-221: ⚡ Quick winStrengthen the concurrency assertion.
maxActive >= 1is guaranteed as soon as the first callback starts, so this still passes if all 8 invocations run serially. If this test is meant to prove interleaving, assert>= 2or rename it to match what it actually checks.🤖 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/sdks/nodejs/bridge_nodejs/tests/host_callable.test.ts` around lines 214 - 221, The current concurrency assertion is too weak (expect(maxActive).toBeGreaterThanOrEqual(1)); update the test to assert actual interleaving by requiring at least two concurrent callbacks: change the assertion to expect(maxActive).toBeGreaterThanOrEqual(2). Locate the block creating promises with callFunction(rt, 'CallCb', ...) and the maxActive assertion in host_callable.test.ts and replace the >=1 check with >=2 (or alternatively rename the test if you intend to only assert that callbacks ran).
🤖 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/baml_builtins2/baml_std/baml/ns_errors/errors.baml`:
- Around line 58-63: Add a message field to the HostCallable BAML error struct
and populate it from the host error when translating OpErrorKind::HostCallable
so the host diagnostic is not dropped; specifically, update the HostCallable
definition to include a "message string" (or "message string?"/nullable if
appropriate for existing callers) and update the code that constructs
baml.errors.HostCallable (the translation path handling
OpErrorKind::HostCallable) to pass OpErrorKind::HostCallable.message into that
new field.
In `@baml_language/crates/bex_engine/src/conversion.rs`:
- Around line 822-863: The Type::Class branch currently accepts
BexExternalValue::Map and returns Ok(()), which lets a Map be pushed into the
VM; change the Ty::Class match arm so that BexExternalValue::Map is treated as a
schema mismatch and returns an Err with a clear message (e.g. "expected instance
of `{tn}` but got Map") instead of Ok(()). Locate the Ty::Class arm in
conversion.rs (the method that calls self.validate_host_return_schema) and
replace the BexExternalValue::Map => Ok(()) case with a return Err(...) that
includes the declared class name (tn) and the fact a Map was returned.
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 2348-2377: The code currently returns EngineError::UnhandledThrow
when op_error_to_catchable_throw yields a value, which causes
run_thread_event_loop to exit and makes HostCallable throws uncaught by in-VM
try/catch; instead, change this path to schedule/propagate the throw via the
same VM-reentry mechanism used by the adjacent await handling so the exception
table runs: do not return EngineError::UnhandledThrow from this spot in lib.rs,
but convert the throw_value into the EngineError variant or call the helper used
by the await arm (the same error-construction/path that run_thread_event_loop
uses for await/future errors) so call_host_value's declared throws
(root.errors.HostCallable) are delivered back into the VM and can be caught.
In `@baml_language/crates/bex_external_types/src/host_return.rs`:
- Around line 190-195: The match currently falls through with `_ => true` in
host_return.rs which permissively accepts function-typed return shapes; replace
the catch-all with explicit match arms and add an explicit branch for the
function-shaped type (e.g., `Ty::Function` or the exact variant name
representing function types in the `Ty` enum) that returns `false` to reject
non-callable values at this boundary, keep allowed variants returning `true`,
and add unit tests for function-typed host returns to assert rejection.
In `@baml_language/crates/bex_resource_types/src/host_value.rs`:
- Around line 224-234: The pending release list can contain duplicates causing
the same key to be released multiple times; modify the collection logic around
INTERNER and pending (where to_release is built) to deduplicate keys before
invoking the release callbacks (e.g., collect into a HashSet or use
.fold/.unique) so each u64 key from pending is only released once; apply the
same deduplication change to the other occurrence around lines 253-257 (the
other drain/release path) and ensure subsequent code still iterates the
deduplicated set when calling the host unregister/release callbacks.
In `@baml_language/crates/bex_vm/src/vm.rs`:
- Around line 4081-4151: CallIndirect currently builds a VmExecState::SysOp for
Object::HostClosure (using BamlHostCallHostValue), but the native callback path
(NativeCallResult::YieldToCall -> execute_call_from_locals_offset) still rejects
HostClosure; factor the callable-dispatch into a shared helper (e.g.,
dispatch_callable_or_sysop) or modify execute_call_from_locals_offset to
recognize Object::HostClosure and return the same VmExecState::SysOp payload
(operation bex_vm_types::SysOp::BamlHostCallHostValue with args: callee handle,
args_array pointer, ret_ty pointer) instead of rejecting it, ensuring the same
arg layout and TLAB allocations (args array and ret_ty) and preserving existing
checks (arity, stack drainage) used in the CallIndirect branch.
In `@baml_language/crates/bridge_cffi/src/ffi/host_value.rs`:
- Around line 89-94: The code currently treats a null `content` pointer with a
non-zero `length` as an empty slice; change the branch that builds `bytes` so
that if `content.is_null()` and `length > 0` you explicitly fail the ABI (e.g.,
return an Err / propagate an error or abort) instead of producing `&[]`. Locate
the `bytes: &[u8]` creation that uses `content`, `length`, and
`std::slice::from_raw_parts` and add a guard: allow `&[]` only when `length ==
0`, otherwise detect `content.is_null()` and return an explicit error describing
the invalid null pointer with non-zero length.
In `@baml_language/crates/sys_native/src/host_dispatch.rs`:
- Around line 121-127: The debug_assert allows silent overwrite in release;
change the insert to a runtime-checked insert so we never replace an existing
in-flight entry. For example, replace table.insert(call_id, completion) with a
checked insert that detects a previous value (e.g., if table.insert(call_id,
completion).is_some() { panic!("host-call id {call_id} collided with a live
in-flight entry; the u32 call-id space wrapped while an entry was still pending
(this should be impossible now that cancelled calls are evicted via
InflightGuard)") }), or alternatively use
table.entry(call_id).or_insert_with(...) with explicit handling when Occupied to
panic or return an error; keep the check tied to call_id, completion and
reference the InflightGuard semantics.
- Around line 72-74: The current use of tokio::task::block_in_place in
fire_dispatch risks panics on current_thread runtimes; replace the
block_in_place call in fire_dispatch with tokio::task::spawn_blocking to run the
blocking callback off the async runtime thread (i.e., call
tokio::task::spawn_blocking(move || { f(host_value_key, call_id, args.as_ptr(),
args.len()); }) and handle the returned JoinHandle result appropriately), or if
fire_dispatch is synchronous and cannot await, document/require a multi-threaded
runtime and assert or return an error instead; update the code paths referencing
fire_dispatch to await the spawn_blocking JoinHandle or propagate errors
accordingly.
In `@baml_language/sdks/go/bridge_go/pkg/host_value.go`:
- Around line 161-163: Add a guard in bamlHostDispatch before calling C.GoBytes:
check that the incoming length (C.size_t) does not exceed the max value
representable by C.int (use math.MaxInt32 or equivalent), and if it does return
a HostCallableError instead of performing the conversion; only call
C.GoBytes(unsafe.Pointer(args), C.int(length)) when length is within bounds and
args != nil, ensuring you avoid C.size_t → C.int truncation and potential unsafe
reads.
In `@baml_language/sdks/nodejs/bridge_nodejs/proto.js`:
- Around line 268-275: The code calls result.then(...) for values passing
isPromiseLike which can be arbitrary thenables; change this to use
Promise.resolve(result).then(...) so the standard promise resolution procedure
enforces single-settlement and consistent error handling. Update the block that
currently checks isPromiseLike(result) and replace the direct result.then(...)
invocation with Promise.resolve(result).then(resolved =>
sendHostCallableResult(callId, resolved), err => sendHostCallableError(callId,
err)), keeping the same callId and handler functions.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts`:
- Around line 295-309: sendHostCallableResult must be made rollback- and
completion-safe: allocate a local registered array (e.g. const registered:
Array<...> = []) and pass it to setInboundValue({ syncMode: false, registered
}), then if setInboundValue/InboundValue.encode throws, call
sendHostCallableError(callId, err) AND iterate the registered array to
release/unregister those host-callable keys (use your existing
cleanup/unregister helper used elsewhere), and finally ensure
completeHostCall(callId, 0, bytes) is invoked in a finally block (wrap the call
in its own try/catch so any native throw cannot escape the promise). Refer to
sendHostCallableResult, setInboundValue, the local registered array,
sendHostCallableError, and completeHostCall when making the changes.
---
Outside diff comments:
In `@baml_language/crates/bridge_wasm/src/handle.rs`:
- Around line 108-111: The Drop impl for BamlHandle currently always calls
HANDLE_TABLE.release(self.key) which incorrectly releases keys that belong to
the host-value registry; change the Drop implementation to first check the
handle kind/variant (the BamlHandle field that indicates HOST_VALUE_CALLABLE)
and skip calling HANDLE_TABLE.release when the handle is HOST_VALUE_CALLABLE
(i.e., return early or match on the variant and only call HANDLE_TABLE.release
for non-HOST_VALUE_CALLABLE cases), referencing the BamlHandle Drop impl, the
HOST_VALUE_CALLABLE constant/variant, and HANDLE_TABLE.release to locate the
code to change.
---
Nitpick comments:
In `@baml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.ts`:
- Around line 214-221: The current concurrency assertion is too weak
(expect(maxActive).toBeGreaterThanOrEqual(1)); update the test to assert actual
interleaving by requiring at least two concurrent callbacks: change the
assertion to expect(maxActive).toBeGreaterThanOrEqual(2). Locate the block
creating promises with callFunction(rt, 'CallCb', ...) and the maxActive
assertion in host_callable.test.ts and replace the >=1 check with >=2 (or
alternatively rename the test if you intend to only assert that callbacks ran).
🪄 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: c95b7e5e-2ffa-45e6-ba12-bac31715416f
⛔ Files ignored due to path filters (74)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__01_lexer__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__02_parser__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__10_formatter__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_basic/baml_tests__compiles__test_expr_basic__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_basic/baml_tests__compiles__test_expr_basic__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_basic/baml_tests__compiles__test_expr_basic__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_basic/baml_tests__compiles__test_expr_basic__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_name_concat/baml_tests__compiles__test_expr_name_concat__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_name_concat/baml_tests__compiles__test_expr_name_concat__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_name_concat/baml_tests__compiles__test_expr_name_concat__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_name_concat/baml_tests__compiles__test_expr_name_concat__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_throwing_body/baml_tests__compiles__test_expr_throwing_body__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_throwing_body/baml_tests__compiles__test_expr_throwing_body__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_throwing_body/baml_tests__compiles__test_expr_throwing_body__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_expr_throwing_body/baml_tests__compiles__test_expr_throwing_body__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_old_and_new/baml_tests__compiles__test_old_and_new__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_old_and_new/baml_tests__compiles__test_old_and_new__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_old_and_new/baml_tests__compiles__test_old_and_new__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_old_and_new/baml_tests__compiles__test_old_and_new__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_raw_string_name/baml_tests__compiles__test_raw_string_name__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_raw_string_name/baml_tests__compiles__test_raw_string_name__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_raw_string_name/baml_tests__compiles__test_raw_string_name__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_raw_string_name/baml_tests__compiles__test_raw_string_name__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_with_not_keyword/baml_tests__compiles__test_with_not_keyword__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_with_not_keyword/baml_tests__compiles__test_with_not_keyword__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_with_not_keyword/baml_tests__compiles__test_with_not_keyword__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/test_with_not_keyword/baml_tests__compiles__test_with_not_keyword__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_basic/baml_tests__compiles__testset_basic__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_basic/baml_tests__compiles__testset_basic__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_basic/baml_tests__compiles__testset_basic__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_basic/baml_tests__compiles__testset_basic__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_dynamic/baml_tests__compiles__testset_dynamic__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_dynamic/baml_tests__compiles__testset_dynamic__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_dynamic/baml_tests__compiles__testset_dynamic__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_dynamic/baml_tests__compiles__testset_dynamic__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_nested/baml_tests__compiles__testset_nested__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_nested/baml_tests__compiles__testset_nested__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_nested/baml_tests__compiles__testset_nested__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_nested/baml_tests__compiles__testset_nested__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_with_setup/baml_tests__compiles__testset_with_setup__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_with_setup/baml_tests__compiles__testset_with_setup__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_with_setup/baml_tests__compiles__testset_with_setup__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_with_setup/baml_tests__compiles__testset_with_setup__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_name_type_error/baml_tests__diagnostic_errors__test_expr_name_type_error__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_name_type_error/baml_tests__diagnostic_errors__test_expr_name_type_error__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_with_runner/baml_tests__diagnostic_errors__test_expr_with_runner__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_with_runner/baml_tests__diagnostic_errors__test_expr_with_runner__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_wrong_runner/baml_tests__diagnostic_errors__test_expr_wrong_runner__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_expr_wrong_runner/baml_tests__diagnostic_errors__test_expr_wrong_runner__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_with_runner_ambiguity/baml_tests__diagnostic_errors__test_with_runner_ambiguity__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/test_with_runner_ambiguity/baml_tests__diagnostic_errors__test_with_runner_ambiguity__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/sdks/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (76)
baml_language/crates/baml_builtins2/baml_std/baml/ns_errors/errors.bamlbaml_language/crates/baml_builtins2/baml_std/baml/ns_host/host.bamlbaml_language/crates/baml_builtins2/src/lib.rsbaml_language/crates/baml_tests/projects/compiles/host_callable_call/main.bamlbaml_language/crates/bex_engine/Cargo.tomlbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/host_value_callable.rsbaml_language/crates/bex_events/src/serialize.rsbaml_language/crates/bex_external_types/Cargo.tomlbaml_language/crates/bex_external_types/src/bex_external_value.rsbaml_language/crates/bex_external_types/src/host_return.rsbaml_language/crates/bex_external_types/src/lib.rsbaml_language/crates/bex_heap/src/accessor.rsbaml_language/crates/bex_heap/src/gc.rsbaml_language/crates/bex_heap/src/heap_debugger/real.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_resource_types/Cargo.tomlbaml_language/crates/bex_resource_types/src/host_value.rsbaml_language/crates/bex_resource_types/src/lib.rsbaml_language/crates/bex_vm/src/package_baml/json.rsbaml_language/crates/bex_vm/src/package_baml/root.rsbaml_language/crates/bex_vm/src/package_baml/unstable.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm_types/src/lib.rsbaml_language/crates/bex_vm_types/src/types.rsbaml_language/crates/bridge_cffi/src/ffi/host_value.rsbaml_language/crates/bridge_cffi/src/ffi/mod.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_ctypes/src/handle_table.rsbaml_language/crates/bridge_ctypes/src/value_decode.rsbaml_language/crates/bridge_ctypes/src/value_encode.rsbaml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_inbound.protobaml_language/crates/bridge_wasm/src/handle.rsbaml_language/crates/bridge_wasm/src/host_value.rsbaml_language/crates/bridge_wasm/src/lib.rsbaml_language/crates/bridge_wasm/tests/host_callable.rsbaml_language/crates/bridge_wasm/tests/wasm_fs_glob.rsbaml_language/crates/sys_llm/src/build_request/mod.rsbaml_language/crates/sys_llm/src/jinja/value_conversion.rsbaml_language/crates/sys_native/Cargo.tomlbaml_language/crates/sys_native/src/host_dispatch.rsbaml_language/crates/sys_native/src/host_impls.rsbaml_language/crates/sys_native/src/lib.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/crates/sys_types/src/lib.rsbaml_language/crates/tools_onionskin/src/compiler.rsbaml_language/sdk_tests/crates/python_pydantic2/host_callables/customizable/test_main.pybaml_language/sdk_tests/fixtures/host_callables/baml_src/main.bamlbaml_language/sdks/go/bridge_go/cffi/bridge.hbaml_language/sdks/go/bridge_go/cffi/lib.gobaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/host_callable.gobaml_language/sdks/go/bridge_go/pkg/host_value.gobaml_language/sdks/go/bridge_go/pkg/host_value_internal_test.gobaml_language/sdks/go/bridge_go/pkg/init_callbacks.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/go/bridge_go/pkg/tests/host_callable_test.gobaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/proto.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/host_value.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/python/rust/bridge_python/src/host_value.rsbaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pybaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pyibaml_language/sdks/python/src/baml_core/proto.pybaml_language/sdks/python/tests/test_host_callable.py
2kai2kai2
force-pushed
the
kai/host-value-resource
branch
from
7c84e78 to
ea19ff6
Compare
Binary size checks passed✅ 7 passed
Generated by |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
baml_language/sdks/go/bridge_go/pkg/host_value.go (1)
161-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
lengthbeforeC.GoBytesto prevent truncation across FFI.On Line 162,
C.size_tis cast toC.intwithout a max-bound check. Largelengthvalues can truncate and cause incorrect/unsafe reads.💡 Suggested fix
var argsBytes []byte if length > 0 && args != nil { + if length > C.size_t(math.MaxInt32) { + sendHostCallableError(id, "DecodeError", + fmt.Sprintf("host-call args buffer too large: %d bytes", uint64(length)), + pb.HostCallableErrorCategory_HOST_CALLABLE_INVALID_ARGUMENT) + return + } argsBytes = C.GoBytes(unsafe.Pointer(args), C.int(length)) }🤖 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/sdks/go/bridge_go/pkg/host_value.go` around lines 161 - 163, The code calls C.GoBytes(unsafe.Pointer(args), C.int(length)) and casts C.size_t length to C.int which can truncate large values; update the guard in the host_value.go snippet that sets argsBytes so you validate length before casting: check that args != nil and length > 0 and that length is <= the maximum representable value of C.int (e.g., INT_MAX) and handle the overflow case (return an error, cap safely, or avoid calling C.GoBytes) so you never pass a truncated C.int to C.GoBytes; reference the variables/functions argsBytes, length, args and the use of C.GoBytes to locate and fix the code.
🧹 Nitpick comments (1)
baml_language/crates/sys_ops/src/lib.rs (1)
1583-1610: ⚡ Quick winAdd a unit test for
with_host_instancedispatch wiring.This new namespace override path should have a focused unit test in this module to ensure
baml_host_call_host_valueis routed through the injectedIoNamespaceHostimplementation.As per coding guidelines "Prefer writing Rust unit tests over integration tests where possible".
🤖 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/sys_ops/src/lib.rs` around lines 1583 - 1610, Add a focused unit test that verifies with_host_instance wires the host dispatch: implement a small test-only struct that implements io::IoNamespaceHost and records when __glue_baml_host_call_host_value is invoked (e.g., via an AtomicBool or channel), call SysOps::builder().with_host_instance(Arc::new(your_impl)) to produce the builder/instance, then invoke the stored closure self.inner.baml_host_call_host_value (or obtain the SysOps and call the equivalent dispatch entry) with dummy heap/permit/args/ctx/call_id and assert your recorder was triggered; reference with_host_instance, baml_host_call_host_value, io::IoNamespaceHost, and __glue_baml_host_call_host_value to locate where to wire the test.
🤖 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/sdks/go/bridge_go/pkg/host_value.go`:
- Around line 304-317: The code in runHostCallable currently treats a two-value
return as an error by inspecting out[1] at runtime; update the case that handles
two returns to first validate the function signature by checking
fnType.Out(1).Implements(errorType) before casting out[1] to error, and if that
check fails sendHostCallableError(callID, "TypeError" or appropriate class,
"host callable second return must implement error",
pb.HostCallableErrorCategory_HOST_CALLABLE_HOST_ERROR, withTraceback("invalid
host callable signature")) and return; otherwise proceed to cast out[1] to error
and handle as before (references: runHostCallable, fnType, out, errorType,
sendHostCallableError, callID, withTraceback).
---
Duplicate comments:
In `@baml_language/sdks/go/bridge_go/pkg/host_value.go`:
- Around line 161-163: The code calls C.GoBytes(unsafe.Pointer(args),
C.int(length)) and casts C.size_t length to C.int which can truncate large
values; update the guard in the host_value.go snippet that sets argsBytes so you
validate length before casting: check that args != nil and length > 0 and that
length is <= the maximum representable value of C.int (e.g., INT_MAX) and handle
the overflow case (return an error, cap safely, or avoid calling C.GoBytes) so
you never pass a truncated C.int to C.GoBytes; reference the variables/functions
argsBytes, length, args and the use of C.GoBytes to locate and fix the code.
---
Nitpick comments:
In `@baml_language/crates/sys_ops/src/lib.rs`:
- Around line 1583-1610: Add a focused unit test that verifies
with_host_instance wires the host dispatch: implement a small test-only struct
that implements io::IoNamespaceHost and records when
__glue_baml_host_call_host_value is invoked (e.g., via an AtomicBool or
channel), call SysOps::builder().with_host_instance(Arc::new(your_impl)) to
produce the builder/instance, then invoke the stored closure
self.inner.baml_host_call_host_value (or obtain the SysOps and call the
equivalent dispatch entry) with dummy heap/permit/args/ctx/call_id and assert
your recorder was triggered; reference with_host_instance,
baml_host_call_host_value, io::IoNamespaceHost, and
__glue_baml_host_call_host_value to locate where to wire the test.
🪄 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: 9b2d2800-1105-47eb-9a73-d61ae5fcf33a
⛔ Files ignored due to path filters (13)
baml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__01_lexer__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__02_parser__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__10_formatter__main.snapis excluded by!**/*.snapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/sdks/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
baml_language/crates/baml_tests/projects/compiles/host_callable_call/main.bamlbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/host_value_callable.rsbaml_language/crates/bex_external_types/src/host_return.rsbaml_language/crates/bex_external_types/src/lib.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_resource_types/src/host_value.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bridge_ctypes/src/value_decode.rsbaml_language/crates/bridge_wasm/src/host_value.rsbaml_language/crates/bridge_wasm/src/lib.rsbaml_language/crates/bridge_wasm/tests/host_callable.rsbaml_language/crates/bridge_wasm/tests/wasm_fs_glob.rsbaml_language/crates/sys_native/src/host_dispatch.rsbaml_language/crates/sys_native/src/host_impls.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/sdk_tests/crates/python_pydantic2/host_callables/customizable/test_main.pybaml_language/sdk_tests/fixtures/host_callables/baml_src/main.bamlbaml_language/sdks/go/bridge_go/cffi/lib.gobaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/host_callable.gobaml_language/sdks/go/bridge_go/pkg/host_value.gobaml_language/sdks/go/bridge_go/pkg/host_value_internal_test.gobaml_language/sdks/go/bridge_go/pkg/init_callbacks.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/go/bridge_go/pkg/tests/host_callable_test.gobaml_language/sdks/nodejs/bridge_nodejs/Cargo.tomlbaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/proto.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/host_value.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/python/rust/bridge_python/src/host_value.rsbaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/src/baml_core/proto.pybaml_language/sdks/python/tests/test_host_callable.py
✅ Files skipped from review due to trivial changes (5)
- baml_language/sdks/nodejs/bridge_nodejs/native.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/native.js
- baml_language/sdks/nodejs/bridge_nodejs/proto.js
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.js
Merging this PR will degrade performance by 12.44%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | vm_string_concat_5k |
24.1 ms | 27.8 ms | -13.58% |
| ❌ | WallTime | vm_class_create_50k |
11.2 ms | 12.6 ms | -11.28% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing kai/host-value-resource (4128115) with canary (a9a1cae)
2kai2kai2
force-pushed
the
kai/host-value-resource
branch
from
ea19ff6 to
93e63b7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
baml_language/crates/sys_ops/src/lib.rs (1)
1583-1610: ⚡ Quick winAdd unit coverage for host builder wiring.
with_host_instance/with_hostadd a new dispatch path; please add a unit test in this module that verifiesbaml_host_call_host_valueis actually overridden and forwards calls as expected.As per coding guidelines "Prefer writing Rust unit tests over integration tests where possible".
🤖 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/sys_ops/src/lib.rs` around lines 1583 - 1610, Add a unit test in this module that constructs the same builder type that exposes with_host_instance/with_host, supplies a test implementation of io::IoNamespaceHost (implementing __glue_baml_host_call_host_value and Default or wrapped in Arc) which records that it was called (e.g. via an Arc<Mutex<...>>), calls the builder.with_host_instance(...) (or with_host::<YourTestImpl>()), then invokes the stored closure inner.baml_host_call_host_value (tests in-module can access private inner) or exercises the public path that triggers it, and assert the recorded arguments/side-effects to prove the inner.baml_host_call_host_value field was overridden and forwards calls to your test impl. Ensure the test lives under #[cfg(test)] and uses the exact symbols with_host_instance, with_host, inner.baml_host_call_host_value, and io::IoNamespaceHost::__glue_baml_host_call_host_value.baml_language/sdks/go/bridge_go/pkg/tests/host_callable_test.go (1)
389-396: 💤 Low valueUse
strings.Containsinstead of manual implementation.The
containshelper reimplementsstrings.Containsfrom the standard library. Using the stdlib function is more idiomatic and avoids maintaining custom code.♻️ Proposed fix
+import ( + "strings" + // ... other imports +) -func contains(haystack, needle string) bool { - for i := 0; i+len(needle) <= len(haystack); i++ { - if haystack[i:i+len(needle)] == needle { - return true - } - } - return false -} +func contains(haystack, needle string) bool { + return strings.Contains(haystack, needle) +}Or simply replace
contains(...)calls withstrings.Contains(...)directly.🤖 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/sdks/go/bridge_go/pkg/tests/host_callable_test.go` around lines 389 - 396, The custom contains(haystack, needle string) function reimplements standard library behavior—replace usages of contains(...) with strings.Contains(...) and remove the contains function; import the "strings" package where needed and ensure any tests referencing contains now call strings.Contains to use the stdlib implementation.baml_language/sdks/go/bridge_go/pkg/proto.go (1)
130-154: 💤 Low valueConsider guarding against nil func registration.
The reflective fallback registers any value with
Kind() == reflect.Func, including typed nil funcs (e.g.,var f func() = nil). Invoking a nil func will panic at dispatch time. If this is intentional (letting the dispatch path surface a clear error), this is fine; otherwise, adding an!rv.IsNil()check would reject nil funcs early with a clearer error message.♻️ Optional: reject nil funcs early
rv := reflect.ValueOf(v) if rv.IsValid() && rv.Kind() == reflect.Func { + if rv.IsNil() { + return nil, fmt.Errorf("nil func cannot be used as a host callable") + } key := registerHostValue(rv)🤖 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/sdks/go/bridge_go/pkg/proto.go` around lines 130 - 154, The code currently registers any reflect.Func, including typed nil funcs (rv.Kind()==reflect.Func) which will panic at dispatch; modify the guard in the reflective fallback to also check that the func value is non-nil (e.g., require rv.IsValid() && rv.Kind()==reflect.Func && !rv.IsNil()) before calling registerHostValue(rv), and if the func is nil return a clear error (instead of registering) so callers see an immediate "nil func" / unsupported value error; update the branch that appends to registered and returns the pb.InboundValue accordingly, leaving registerHostValue and the pb.InboundValue construction unchanged.baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts (1)
296-311: 💤 Low valueMinor: Result-encode path lacks rollback for nested callables.
The
registered: []array passed tosetInboundValueis created inline and discarded if encoding fails. If a host callable returns a value containing another callable and encoding fails after registering that inner callable, the key leaks (the engine never sees it to release).This is a narrow edge case (callable returning callable, then encode failure), but for completeness the fix would be to hoist
ctxand iteratectx.registeredin the catch block, similar toencodeCallArgs.♻️ Optional fix for completeness
function sendHostCallableResult(callId: number, value: unknown): void { - let bytes: Buffer; + const ctx: EncodeCtx = { syncMode: false, registered: [] }; try { const iv: baml_core.cffi.v1.IInboundValue = {}; - // Result-encode path (host → engine): no sync guard (we're already - // on libuv) and no rollback tracking — the result is handed to the - // engine, which releases any callables it decodes. - setInboundValue(iv, value, { syncMode: false, registered: [] }); + setInboundValue(iv, value, ctx); const msg = InboundValue.create(iv); - bytes = Buffer.from(InboundValue.encode(msg).finish()); + const bytes = Buffer.from(InboundValue.encode(msg).finish()); + completeHostCall(callId, 0, bytes); } catch (err) { + for (const key of ctx.registered) { + try { releaseHostCallable(key); } catch { /* best-effort */ } + } sendHostCallableError(callId, err); - return; } - completeHostCall(callId, 0, bytes); }🤖 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/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts` around lines 296 - 311, sendHostCallableResult creates its registered array inline so if setInboundValue registers nested callables then encoding fails those keys are leaked; hoist the inbound context (e.g., const ctx: baml_core.cffi.v1.IInboundValue = { registered: [] }) and pass ctx to setInboundValue, and in the catch block iterate ctx.registered to release/unregister any registered callables (matching the pattern used in encodeCallArgs) before calling sendHostCallableError; update references to use ctx (or iv) consistently and ensure completeHostCall is only called on success.baml_language/sdks/nodejs/bridge_nodejs/src/host_value.rs (1)
96-116: 💤 Low valueInconsistent mutex poisoning handling.
register_host_callableuses.unwrap()on the mutex lock (line 114), which will panic if a previous thread panicked while holding the lock. However,drop_registry_entry(line 147) andhost_dispatch_callback(line 211) both handle the poisoned case gracefully. For consistency and robustness, consider handling the poisoned case here as well.Proposed fix
pub fn register_host_callable(callable: Function<'_, DispatchArgs, ()>) -> napi::Result<HandleKey> { let tsfn: DispatchTsfn = callable .build_threadsafe_function() .callee_handled::<false>() .weak::<false>() .build()?; let key = next_key(); - REGISTRY.table.lock().unwrap().insert(key, Arc::new(tsfn)); + match REGISTRY.table.lock() { + Ok(mut t) => { t.insert(key, Arc::new(tsfn)); } + Err(poisoned) => { poisoned.into_inner().insert(key, Arc::new(tsfn)); } + } Ok(HandleKey::from_u64(key)) }🤖 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/sdks/nodejs/bridge_nodejs/src/host_value.rs` around lines 96 - 116, The mutex lock in register_host_callable currently uses .unwrap() on REGISTRY.table.lock(), which can panic on a poisoned mutex; change it to handle the poisoned case like the other functions (drop_registry_entry and host_dispatch_callback) by matching the Result from lock() and calling into_inner() on the PoisonError (or using unwrap_or_else(|e| e.into_inner())) to recover the inner guard, then insert the Arc<DispatchTsfn> as before; keep references to next_key(), REGISTRY.table, and HandleKey usage intact.baml_language/sdks/python/rust/bridge_python/src/host_value.rs (1)
71-122: 💤 Low valueInconsistent mutex poisoning handling.
register_host_callableuses.unwrap()on the mutex lock (line 79), which will panic if a previous thread panicked while holding the lock. However,drop_registry_entry(line 91) handles the poisoned case gracefully by returning early. For consistency, consider handling the poisoned case here as well.Proposed fix
pub fn register_host_callable(callable: Py<PyAny>) -> u64 { let key = next_key(); - REGISTRY.table.lock().unwrap().insert(key, callable); + match REGISTRY.table.lock() { + Ok(mut t) => { t.insert(key, callable); } + Err(poisoned) => { poisoned.into_inner().insert(key, callable); } + } key }🤖 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/sdks/python/rust/bridge_python/src/host_value.rs` around lines 71 - 122, The register_host_callable function currently uses REGISTRY.table.lock().unwrap() which will panic on a poisoned mutex; make it handle the poisoned case like drop_registry_entry does by replacing the unwrap with a match on REGISTRY.table.lock() and if Err(_) return the generated key without inserting (or otherwise no-op) to avoid panics; specifically update register_host_callable to use match REGISTRY.table.lock() { Ok(mut t) => t.insert(key, callable), Err(_) => return key } so behavior is consistent with drop_registry_entry.
🤖 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 1802-1815: The host-closure branch currently skips runtime arity
checks and drains the stack immediately; change it to validate arg_count against
the HostClosure's expected arity before touching the stack and return
Err(InvalidArgumentCount) on mismatch. Concretely, when matching
self.get_object(callee_ptr) to Object::HostClosure(hc) extract the closure's
arity from hc (or its field), compare it to arg_count, and only if they match
call self.host_closure_call_sysop(callee_ptr, user_args) after draining via
self.stack.drain(locals_offset..); otherwise return the InvalidArgumentCount
error without draining.
---
Nitpick comments:
In `@baml_language/crates/sys_ops/src/lib.rs`:
- Around line 1583-1610: Add a unit test in this module that constructs the same
builder type that exposes with_host_instance/with_host, supplies a test
implementation of io::IoNamespaceHost (implementing
__glue_baml_host_call_host_value and Default or wrapped in Arc) which records
that it was called (e.g. via an Arc<Mutex<...>>), calls the
builder.with_host_instance(...) (or with_host::<YourTestImpl>()), then invokes
the stored closure inner.baml_host_call_host_value (tests in-module can access
private inner) or exercises the public path that triggers it, and assert the
recorded arguments/side-effects to prove the inner.baml_host_call_host_value
field was overridden and forwards calls to your test impl. Ensure the test lives
under #[cfg(test)] and uses the exact symbols with_host_instance, with_host,
inner.baml_host_call_host_value, and
io::IoNamespaceHost::__glue_baml_host_call_host_value.
In `@baml_language/sdks/go/bridge_go/pkg/proto.go`:
- Around line 130-154: The code currently registers any reflect.Func, including
typed nil funcs (rv.Kind()==reflect.Func) which will panic at dispatch; modify
the guard in the reflective fallback to also check that the func value is
non-nil (e.g., require rv.IsValid() && rv.Kind()==reflect.Func && !rv.IsNil())
before calling registerHostValue(rv), and if the func is nil return a clear
error (instead of registering) so callers see an immediate "nil func" /
unsupported value error; update the branch that appends to registered and
returns the pb.InboundValue accordingly, leaving registerHostValue and the
pb.InboundValue construction unchanged.
In `@baml_language/sdks/go/bridge_go/pkg/tests/host_callable_test.go`:
- Around line 389-396: The custom contains(haystack, needle string) function
reimplements standard library behavior—replace usages of contains(...) with
strings.Contains(...) and remove the contains function; import the "strings"
package where needed and ensure any tests referencing contains now call
strings.Contains to use the stdlib implementation.
In `@baml_language/sdks/nodejs/bridge_nodejs/src/host_value.rs`:
- Around line 96-116: The mutex lock in register_host_callable currently uses
.unwrap() on REGISTRY.table.lock(), which can panic on a poisoned mutex; change
it to handle the poisoned case like the other functions (drop_registry_entry and
host_dispatch_callback) by matching the Result from lock() and calling
into_inner() on the PoisonError (or using unwrap_or_else(|e| e.into_inner())) to
recover the inner guard, then insert the Arc<DispatchTsfn> as before; keep
references to next_key(), REGISTRY.table, and HandleKey usage intact.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts`:
- Around line 296-311: sendHostCallableResult creates its registered array
inline so if setInboundValue registers nested callables then encoding fails
those keys are leaked; hoist the inbound context (e.g., const ctx:
baml_core.cffi.v1.IInboundValue = { registered: [] }) and pass ctx to
setInboundValue, and in the catch block iterate ctx.registered to
release/unregister any registered callables (matching the pattern used in
encodeCallArgs) before calling sendHostCallableError; update references to use
ctx (or iv) consistently and ensure completeHostCall is only called on success.
In `@baml_language/sdks/python/rust/bridge_python/src/host_value.rs`:
- Around line 71-122: The register_host_callable function currently uses
REGISTRY.table.lock().unwrap() which will panic on a poisoned mutex; make it
handle the poisoned case like drop_registry_entry does by replacing the unwrap
with a match on REGISTRY.table.lock() and if Err(_) return the generated key
without inserting (or otherwise no-op) to avoid panics; specifically update
register_host_callable to use match REGISTRY.table.lock() { Ok(mut t) =>
t.insert(key, callable), Err(_) => return key } so behavior is consistent with
drop_registry_entry.
🪄 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: 4257afa5-7d53-48d8-b024-4d8bbdfcae22
⛔ Files ignored due to path filters (19)
baml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__01_lexer__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__02_parser__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__10_formatter__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/sdks/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
baml_language/crates/baml_builtins2/baml_std/baml/ns_errors/errors.bamlbaml_language/crates/baml_tests/projects/compiles/host_callable_call/main.bamlbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/host_value_callable.rsbaml_language/crates/bex_external_types/src/host_return.rsbaml_language/crates/bex_external_types/src/lib.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_resource_types/src/host_value.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bridge_cffi/src/ffi/host_value.rsbaml_language/crates/bridge_ctypes/src/value_decode.rsbaml_language/crates/bridge_wasm/src/host_value.rsbaml_language/crates/bridge_wasm/src/lib.rsbaml_language/crates/bridge_wasm/tests/host_callable.rsbaml_language/crates/bridge_wasm/tests/wasm_fs_glob.rsbaml_language/crates/sys_native/src/host_dispatch.rsbaml_language/crates/sys_native/src/host_impls.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/sdk_tests/crates/python_pydantic2/host_callables/customizable/test_main.pybaml_language/sdk_tests/fixtures/host_callables/baml_src/main.bamlbaml_language/sdks/go/bridge_go/cffi/lib.gobaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/host_callable.gobaml_language/sdks/go/bridge_go/pkg/host_value.gobaml_language/sdks/go/bridge_go/pkg/host_value_internal_test.gobaml_language/sdks/go/bridge_go/pkg/init_callbacks.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/go/bridge_go/pkg/tests/host_callable_test.gobaml_language/sdks/nodejs/bridge_nodejs/Cargo.tomlbaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/proto.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/host_value.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/python/rust/bridge_python/src/host_value.rsbaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/src/baml_core/proto.pybaml_language/sdks/python/tests/test_host_callable.pytypescript2/app-vscode-webview/src/bridge_wasm.test.ts
✅ Files skipped from review due to trivial changes (6)
- baml_language/sdks/nodejs/bridge_nodejs/Cargo.toml
- baml_language/sdks/nodejs/bridge_nodejs/native.js
- baml_language/crates/baml_tests/projects/compiles/host_callable_call/main.baml
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/proto.js
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.js
2kai2kai2
force-pushed
the
kai/host-value-resource
branch
from
93e63b7 to
e16ed99
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
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/bex_engine/src/lib.rs (1)
2303-2345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the declared
ret_tywhen materializing successful host-call results.
validate_host_return_schemacan now acceptBexExternalValue::HostValueforTy::Function, but Line 2345 still goes throughconvert_external_to_vm_value(...), which passesexpected_ty = None. A host callable declared as returning another callable will therefore pass validation and then fail withCannotConvert("host_value (no declared function type in context)").Suggested fix
- let value = self.convert_external_to_vm_value(&mut thread, external)?; + let declared_ret_ty = + (operation == SysOp::BamlHostCallHostValue) + .then(|| host_call_return_ty(args.get(2).copied())) + .flatten(); + let value = self.convert_external_to_vm_value_with_ty( + &mut thread, + external, + declared_ret_ty.as_ref(), + )?; thread.vm.stack.push(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` around lines 2303 - 2345, The code validates host-call returns with validate_host_return_schema using the declared return type (ret_ty) but then calls convert_external_to_vm_value with expected_ty = None, causing host-returned HostValue for function types to fail materialization; update the SysOp::BamlHostCallHostValue path so that when host_call_return_ty(args.get(2).copied()) yields Some(ret_ty) you pass that ret_ty (or a clone/owned copy) as the expected_ty argument into convert_external_to_vm_value (i.e., call convert_external_to_vm_value(&mut thread, external, Some(ret_ty)) or the equivalent API) so HostValue/BexExternalValue::HostValue is materialized according to the declared Ty::Function; keep existing error/throw handling intact.baml_language/crates/bridge_wasm/src/lib.rs (1)
162-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
host_dispatchoptional in the WASM callback bundle
WasmCallbacks’ TypeScript type markshost_dispatchas required, andBamlWasmRuntime::create()unconditionally callscallbacks.host_dispatch()and always injectsWasmHostviawith_host_instance(...). That’s a breaking change for callers that never use host callables (the crate’s own usage example omitshost_dispatch). Keephost_dispatchoptional and only construct/inject the host path when it’s present; otherwise leave the host namespace unsupported. If the wasm-bindgen structural getter can’t returnOption<Function>, probe the JS object withReflect::get/is_undefinedbefore wiring.with_host_instance(...).🤖 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_wasm/src/lib.rs` around lines 162 - 173, The TS type for WasmCallbacks currently requires host_dispatch and BamlWasmRuntime::create unconditionally calls callbacks.host_dispatch() and uses with_host_instance(WasmHost) which breaks callers that omit host_dispatch; update the wiring so host_dispatch is optional: make the wasm-bindgen binding or runtime probe the JS callbacks object for host_dispatch (e.g., via Reflect::get / is_undefined if the getter can't return Option) and only call callbacks.host_dispatch() and call .with_host_instance(WasmHost) when host_dispatch is present; otherwise skip injecting the host (leave the host namespace unsupported) so WasmCallbacks and BamlWasmRuntime::create handle absent host_dispatch safely.
♻️ Duplicate comments (2)
baml_language/crates/bex_vm/src/vm.rs (1)
1802-1815:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
HostClosurearity validation in the indirect callback path.Line 1812 still dispatches
Object::HostClosurewithout checkingarg_countagainst the closure's declared arity, so aYieldToCallproducer can forward the wrong number of args in release builds instead of failing withInvalidArgumentCount.Suggested fix
- if matches!(self.get_object(callee_ptr), Object::HostClosure(_)) { - let user_args: Vec<Value> = self.stack.drain(locals_offset..).collect(); - return Ok(Some(self.host_closure_call_sysop(callee_ptr, user_args))); + if let Object::HostClosure(hc) = self.get_object(callee_ptr) { + if arg_count != hc.arity { + return Err(VmInternalError::InvalidArgumentCount { + expected: hc.arity, + got: arg_count, + } + .into()); + } + let user_args: Vec<Value> = self.stack.drain(locals_offset..).collect(); + return Ok(Some(self.host_closure_call_sysop(callee_ptr, user_args))); }🤖 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 1802 - 1815, The indirect host-callback path currently calls HostClosure without checking arg_count, so validate the requested arg_count against the HostClosure's declared arity before draining the stack or yielding: match self.get_object(callee_ptr) to extract the HostClosure (e.g., HostClosure(closure)), compare closure.arity (or closure.arity()) with the local arg_count and if they differ return the InvalidArgumentCount error; only if they match drain the stack from locals_offset into user_args and call self.host_closure_call_sysop(callee_ptr, user_args) as before.baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts (1)
296-310:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
sendHostCallableResultis still not rollback/completion-safe.This still has the two problems called out earlier: result encoding can register nested host callables and then leak them on a later encode failure, and
completeHostCall(callId, 0, bytes)still sits outside thetry, so a native throw on the promise-fulfillment path can escape the.then(...)handler and leave the call uncompleted.Suggested fix
function sendHostCallableResult(callId: number, value: unknown): void { - let bytes: Buffer; + const ctx: EncodeCtx = { syncMode: false, registered: [] }; try { const iv: baml_core.cffi.v1.IInboundValue = {}; - setInboundValue(iv, value, { syncMode: false, registered: [] }); + setInboundValue(iv, value, ctx); const msg = InboundValue.create(iv); - bytes = Buffer.from(InboundValue.encode(msg).finish()); + const bytes = Buffer.from(InboundValue.encode(msg).finish()); + completeHostCall(callId, 0, bytes); } catch (err) { + for (const key of ctx.registered) { + try { + releaseHostCallable(key); + } catch { + // Best-effort cleanup. + } + } sendHostCallableError(callId, err); - return; } - completeHostCall(callId, 0, bytes); }🤖 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/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts` around lines 296 - 310, The sendHostCallableResult function can leak nested host callables and may leave the call uncompleted if encoding or completion throws; to fix, create and pass a local registered array to setInboundValue (e.g., const registered: number[] = []; setInboundValue(iv, value, { syncMode: false, registered })), wrap the entire encode+complete sequence in a single try/catch so both InboundValue.encode and completeHostCall(callId, 0, bytes) are inside the try, and on any catch call sendHostCallableError(callId, err) and then roll back by unregistering any ids recorded in registered (use the existing host-unregister helper used elsewhere). Ensure no native throw can escape the handler and that registered callables are cleaned up on any error.
🧹 Nitpick comments (4)
baml_language/crates/bridge_ctypes/src/value_decode.rs (1)
44-50: ⚡ Quick winAssert Arc identity in the regression test.
The important behavior change here is shared identity/refcounting, but the test only checks
keyandkind. Re-decoding the same handle twice and asserting pointer equality would catch a regression back to constructing freshHostValueArcs.As per coding guidelines, "Prefer writing Rust unit tests over integration tests where possible".
🤖 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_ctypes/src/value_decode.rs` around lines 44 - 50, Add an assertion in the unit test that re-decodes the same handle and verifies the two HostValueArc instances are the same Arc identity: after calling bex_project::HostValueArc::intern (or whatever helper decodes the wire key) twice for the same wire key/handle, use Arc::ptr_eq to assert the returned Arcs share the same pointer (and optionally check strong_count increased) so the test fails if fresh HostValueArc instances are constructed instead of interned ones.baml_language/crates/bridge_wasm/tests/host_callable.rs (1)
408-422: ⚡ Quick winMake the error-path test assert the surfaced host error.
result.is_err()will also pass for unrelated runtime/setup failures, so this test doesn't actually pin theHostCallableErrormapping added in this PR. Assert that the thrownJsValueincludestest boomorRuntimeError.🤖 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_wasm/tests/host_callable.rs` around lines 408 - 422, Update the test host_callable_error_surfaces_as_baml_error to assert the actual surfaced host error instead of only checking result.is_err(): after calling call_function(...).await, unwrap the Err (e.g. let err = result.unwrap_err()), convert the error/js value to a string (or format!("{:?}", err)) and assert that the message contains either "test boom" or "RuntimeError" so the failure specifically verifies the HostCallableError mapping added by make_dispatch_error / register_host_callable.baml_language/crates/bridge_cffi/src/ffi/host_value.rs (1)
89-108: ⚡ Quick winAdd a unit test for the null-pointer ABI violation path.
This guard is the only thing preventing
(content = NULL, length > 0)from regressing back to the old “treat it as empty” behavior, but the current tests never hit that branch. A small unit test here would lock in the fix.As per coding guidelines, "Prefer writing Rust unit tests over integration tests where possible".
🤖 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/ffi/host_value.rs` around lines 89 - 108, Add a unit test in the same module (host_value.rs) that exercises the null-pointer ABI guard: call the function that contains this code path (the complete_host_call / host-value handling function shown) with content = std::ptr::null() and length > 0 and a test call_id, then assert that host_dispatch::complete_with_error was invoked with an OpError whose op is SysOp::BamlHostCallHostValue and whose message contains "null content pointer" (or the formatted "null content pointer with length {length}") to lock in the guard behavior and prevent regression.baml_language/sdks/nodejs/bridge_nodejs/Cargo.toml (1)
11-16: 🏗️ Heavy liftPreserve a Rust-unit-testable path for this crate.
Disabling both
testanddoctestremoves Rust-level checks for newly added registry/dispatch logic and pushes all validation to external Jest tests. Consider adding a testable Rust target path (e.g., anrlib/split testable module) socargo test --libremains meaningful for Rust changes in this crate.As per coding guidelines "
**/*.rs: Prefer writing Rust unit tests over integration tests where possible" and "Always runcargo test --libif you changed any Rust code`".🤖 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/sdks/nodejs/bridge_nodejs/Cargo.toml` around lines 11 - 16, The crate currently disables Rust-level tests by setting "test = false" and "doctest = false" in Cargo.toml which prevents running cargo test --lib; restore a Rust-testable path by either removing those flags or declaring a proper library target so unit tests can run: edit Cargo.toml to remove or scope the "test" and "doctest" false flags and/or add a [lib] section (crate-type/path) so the crate builds as an rlib and supports #[cfg(test)] unit tests in your Rust modules (e.g., add a [lib] with path = "src/lib.rs" or re-enable doctests) ensuring cargo test --lib exercises the new registry/dispatch logic.
🤖 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/conversion.rs`:
- Around line 796-915: The validator currently allows
BexExternalValue::HostValue to appear inside container and class-field positions
(in validate_host_return_schema), which later fails in
convert_external_to_vm_value; update the Ty::List, Ty::Map and Ty::Class
branches in validate_host_return_schema to explicitly reject HostValue when the
expected nested type is a function (match inner / v_ty / class_field.field_type
against Ty::Function) and return a clear Err for nested host callables;
reference BexExternalValue::HostValue, Ty::Function,
validate_host_return_schema, and convert_external_to_vm_value so the failure is
surfaced here until nested typed conversion is implemented.
In `@baml_language/crates/bridge_wasm/src/host_value.rs`:
- Around line 158-183: The insert_in_flight function currently swallows
collisions by completing the incoming CompletionHandle but returns () so callers
(e.g., call_host_value) still proceed to dispatch the host call; change
insert_in_flight to return a boolean or Result indicating whether the insert
succeeded (e.g., Ok(true) or Ok(()), Err/false on collision), update its
signature and all call sites (including the other occurrence around the 417-455
region) to check the return and immediately return/abort when a collision is
reported, ensuring host_dispatch / call3 is NOT invoked after insert_in_flight
signals failure; keep the existing error-completion behavior inside
insert_in_flight and only change control flow via the new return value.
In `@baml_language/crates/sys_native/src/host_dispatch.rs`:
- Around line 129-154: The insert function currently swallows the collision by
completing the provided CompletionHandle and returning (), so callers like
call_host_value cannot stop before fire_dispatch; change insert(call_id: u32,
completion: CompletionHandle) to return a Result<(), OpError> (or a bool/Result
indicating success) and on collision keep completing the new CompletionHandle
with the OpError::new(SysOp::BamlHostCallHostValue, ...) but then return
Err(...) instead of returning unit; update callers (e.g. call_host_value) to
check the returned Result and bail out (skip firing the host callback) when
insert signals a collision so a rejected call cannot proceed to fire_dispatch.
Ensure symbols referenced: insert, call_host_value, CompletionHandle, TABLE,
OpError, SysOp::BamlHostCallHostValue, InflightGuard.
In `@baml_language/sdks/go/bridge_go/pkg/proto.go`:
- Around line 141-152: The current block treats typed nil funcs as valid and
registers them; change the encode-time validation so that when rv.IsValid() &&
rv.Kind() == reflect.Func you first check rv.IsNil() and return a clear
validation error instead of calling registerHostValue and returning a
pb.InboundValue handle; ensure registerHostValue is not invoked for nil function
values (the symbols to update are the rv.IsValid()/rv.Kind() branch, the
rv.IsNil() check, and the registerHostValue call that produces the
pb.InboundValue/BamlHandle).
In `@baml_language/sdks/nodejs/bridge_nodejs/src/host_value.rs`:
- Around line 107-113: The threadsafe function created in register_host_callable
(the tsfn assigned from callable.build_threadsafe_function()) currently has no
max_queue_size set so its default is unbounded; change the build call chain to
set a sensible max_queue_size (e.g., 256 or 1024) on the ThreadsafeFunction
builder before .build() so the DispatchTsfn created by
build_threadsafe_function() cannot accumulate an unlimited queue while using the
NonBlocking dispatcher; keep the rest of the builder options intact and ensure
the chosen limit prevents unbounded memory growth.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts`:
- Around line 68-89: The branch that handles value instanceof BamlHandle must
also reject HOST_VALUE_CALLABLE on the sync path: inside that branch (where you
set iv.handle = { key: value.key, handleType: value.handleType }), check
ctx.syncMode and if value.handleType === BamlHandleType.HOST_VALUE_CALLABLE
throw the same HostCallableSyncError used for raw functions; this ensures
callable handles are fast-failed on callFunctionSync just like registered JS
callables.
In `@baml_language/sdks/python/rust/bridge_python/src/host_value.rs`:
- Around line 305-315: encode_result_inbound currently calls
proto._set_inbound_value without the rollback accumulator used in
encode_call_args, so any callbacks registered during encoding may leak when
encoding fails; fix by mirroring encode_call_args: create a Py list (e.g.
pyo3::types::PyList::empty(py)) named registered, set it into kwargs_dict with
key "registered" before calling _set_inbound_value, and if any step returns Err
(before successfully returning the bytes) iterate the registered list and
unregister/release each entry using the same cleanup logic used by
encode_call_args, then return the error.
In `@baml_language/sdks/python/tests/test_host_callable.py`:
- Around line 243-255: The test test_encode_success_does_not_release leaves a
real host-value key registered because encode_call_args registers a callable but
the encoded bytes are not sent and release_host_callable is monkeypatched to
only record releases; to fix, either monkeypatch/stub
_proto.register_host_callable to return a fake key (so no real registration
occurs) or call _proto.register_host_callable to capture the real key from
encode_call_args's output and then invoke the real _proto.release_host_callable
in teardown to remove it; reference encode_call_args, register_host_callable and
release_host_callable when making the change.
---
Outside diff comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 2303-2345: The code validates host-call returns with
validate_host_return_schema using the declared return type (ret_ty) but then
calls convert_external_to_vm_value with expected_ty = None, causing
host-returned HostValue for function types to fail materialization; update the
SysOp::BamlHostCallHostValue path so that when
host_call_return_ty(args.get(2).copied()) yields Some(ret_ty) you pass that
ret_ty (or a clone/owned copy) as the expected_ty argument into
convert_external_to_vm_value (i.e., call convert_external_to_vm_value(&mut
thread, external, Some(ret_ty)) or the equivalent API) so
HostValue/BexExternalValue::HostValue is materialized according to the declared
Ty::Function; keep existing error/throw handling intact.
In `@baml_language/crates/bridge_wasm/src/lib.rs`:
- Around line 162-173: The TS type for WasmCallbacks currently requires
host_dispatch and BamlWasmRuntime::create unconditionally calls
callbacks.host_dispatch() and uses with_host_instance(WasmHost) which breaks
callers that omit host_dispatch; update the wiring so host_dispatch is optional:
make the wasm-bindgen binding or runtime probe the JS callbacks object for
host_dispatch (e.g., via Reflect::get / is_undefined if the getter can't return
Option) and only call callbacks.host_dispatch() and call
.with_host_instance(WasmHost) when host_dispatch is present; otherwise skip
injecting the host (leave the host namespace unsupported) so WasmCallbacks and
BamlWasmRuntime::create handle absent host_dispatch safely.
---
Duplicate comments:
In `@baml_language/crates/bex_vm/src/vm.rs`:
- Around line 1802-1815: The indirect host-callback path currently calls
HostClosure without checking arg_count, so validate the requested arg_count
against the HostClosure's declared arity before draining the stack or yielding:
match self.get_object(callee_ptr) to extract the HostClosure (e.g.,
HostClosure(closure)), compare closure.arity (or closure.arity()) with the local
arg_count and if they differ return the InvalidArgumentCount error; only if they
match drain the stack from locals_offset into user_args and call
self.host_closure_call_sysop(callee_ptr, user_args) as before.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts`:
- Around line 296-310: The sendHostCallableResult function can leak nested host
callables and may leave the call uncompleted if encoding or completion throws;
to fix, create and pass a local registered array to setInboundValue (e.g., const
registered: number[] = []; setInboundValue(iv, value, { syncMode: false,
registered })), wrap the entire encode+complete sequence in a single try/catch
so both InboundValue.encode and completeHostCall(callId, 0, bytes) are inside
the try, and on any catch call sendHostCallableError(callId, err) and then roll
back by unregistering any ids recorded in registered (use the existing
host-unregister helper used elsewhere). Ensure no native throw can escape the
handler and that registered callables are cleaned up on any error.
---
Nitpick comments:
In `@baml_language/crates/bridge_cffi/src/ffi/host_value.rs`:
- Around line 89-108: Add a unit test in the same module (host_value.rs) that
exercises the null-pointer ABI guard: call the function that contains this code
path (the complete_host_call / host-value handling function shown) with content
= std::ptr::null() and length > 0 and a test call_id, then assert that
host_dispatch::complete_with_error was invoked with an OpError whose op is
SysOp::BamlHostCallHostValue and whose message contains "null content pointer"
(or the formatted "null content pointer with length {length}") to lock in the
guard behavior and prevent regression.
In `@baml_language/crates/bridge_ctypes/src/value_decode.rs`:
- Around line 44-50: Add an assertion in the unit test that re-decodes the same
handle and verifies the two HostValueArc instances are the same Arc identity:
after calling bex_project::HostValueArc::intern (or whatever helper decodes the
wire key) twice for the same wire key/handle, use Arc::ptr_eq to assert the
returned Arcs share the same pointer (and optionally check strong_count
increased) so the test fails if fresh HostValueArc instances are constructed
instead of interned ones.
In `@baml_language/crates/bridge_wasm/tests/host_callable.rs`:
- Around line 408-422: Update the test
host_callable_error_surfaces_as_baml_error to assert the actual surfaced host
error instead of only checking result.is_err(): after calling
call_function(...).await, unwrap the Err (e.g. let err = result.unwrap_err()),
convert the error/js value to a string (or format!("{:?}", err)) and assert that
the message contains either "test boom" or "RuntimeError" so the failure
specifically verifies the HostCallableError mapping added by make_dispatch_error
/ register_host_callable.
In `@baml_language/sdks/nodejs/bridge_nodejs/Cargo.toml`:
- Around line 11-16: The crate currently disables Rust-level tests by setting
"test = false" and "doctest = false" in Cargo.toml which prevents running cargo
test --lib; restore a Rust-testable path by either removing those flags or
declaring a proper library target so unit tests can run: edit Cargo.toml to
remove or scope the "test" and "doctest" false flags and/or add a [lib] section
(crate-type/path) so the crate builds as an rlib and supports #[cfg(test)] unit
tests in your Rust modules (e.g., add a [lib] with path = "src/lib.rs" or
re-enable doctests) ensuring cargo test --lib exercises the new
registry/dispatch logic.
🪄 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: 09b8e402-d636-40b4-b078-2d5d843590d9
⛔ Files ignored due to path filters (21)
baml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__01_lexer__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__02_parser__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/host_callable_call/baml_tests__compiles__host_callable_call__10_formatter__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snapbaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_inbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_outbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/sdks/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
baml_language/crates/baml_builtins2/baml_std/baml/ns_errors/errors.bamlbaml_language/crates/baml_tests/projects/compiles/host_callable_call/main.bamlbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/host_value_callable.rsbaml_language/crates/bex_external_types/src/host_return.rsbaml_language/crates/bex_external_types/src/lib.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_resource_types/src/host_value.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bridge_cffi/src/ffi/host_value.rsbaml_language/crates/bridge_ctypes/src/value_decode.rsbaml_language/crates/bridge_wasm/src/host_value.rsbaml_language/crates/bridge_wasm/src/lib.rsbaml_language/crates/bridge_wasm/tests/host_callable.rsbaml_language/crates/bridge_wasm/tests/wasm_fs_glob.rsbaml_language/crates/sys_native/src/host_dispatch.rsbaml_language/crates/sys_native/src/host_impls.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/sdk_tests/crates/python_pydantic2/host_callables/customizable/test_main.pybaml_language/sdk_tests/fixtures/host_callables/baml_src/main.bamlbaml_language/sdks/go/bridge_go/cffi/lib.gobaml_language/sdks/go/bridge_go/pkg/host_value.gobaml_language/sdks/go/bridge_go/pkg/host_value_internal_test.gobaml_language/sdks/go/bridge_go/pkg/init_callbacks.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/go/bridge_go/pkg/tests/host_callable_test.gobaml_language/sdks/nodejs/bridge_nodejs/Cargo.tomlbaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/proto.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/host_value.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/python/rust/bridge_python/src/host_value.rsbaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/src/baml_core/proto.pybaml_language/sdks/python/tests/test_host_callable.pytypescript2/app-vscode-webview/src/bridge_wasm.test.ts
✅ Files skipped from review due to trivial changes (4)
- baml_language/crates/bridge_wasm/tests/wasm_fs_glob.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/proto.js
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.js
and other LLM-found fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/sdks/go/bridge_go/pkg/host_value.go (1)
356-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback nested host-value registrations if success completion panics.
goToInboundValueTrackingcan register host callables insideresultValue. IfcompleteHostCallSuccesspanics, the outer recover switches to the error path, butregisteredis never released because the engine never decoded the success payload.Suggested fix
resultBytes, err := proto.Marshal(iv) if err != nil { rollbackRegisteredHostValues(registered) sendHostCallableError(callID, "EncodeError", fmt.Sprintf("failed to marshal host-call result: %v", err), pb.HostCallableErrorCategory_HOST_CALLABLE_HOST_ERROR) return } - completeHostCallSuccess(callID, resultBytes) + func() { + defer func() { + if r := recover(); r != nil { + rollbackRegisteredHostValues(registered) + panic(r) + } + }() + completeHostCallSuccess(callID, resultBytes) + }() }🤖 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/sdks/go/bridge_go/pkg/host_value.go` around lines 356 - 380, The code registers host-callables in `registered` via `goToInboundValueTracking` but never ensures those registrations are rolled back if `completeHostCallSuccess` panics; add a defer immediately after `goToInboundValueTracking` that checks if `registered` is non-nil and calls `rollbackRegisteredHostValues(registered)` (and leaves `sendHostCallableError` behavior unchanged), and after the successful `completeHostCallSuccess(callID, resultBytes)` set `registered = nil` (or otherwise mark it cleared) so the defer does not roll back on normal success; this ensures registrations are cleaned up on panic while not undoing a real success.
♻️ Duplicate comments (1)
baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts (1)
310-336:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
completeHostCallinside the defended success path.
sendHostCallableResultstill callscompleteHostCall(callId, 0, bytes)after thetry/catch. If that native call throws in the promise-fulfillment branch, the rejection bypasses the last-resort completion path, and any keys recorded inctx.registeredstay leaked because cleanup only runs incatch.Suggested fix
function sendHostCallableResult(callId: number, value: unknown): void { - let bytes: Buffer; // Result-encode path (host → engine): no sync guard (we're already on // libuv). We do track registrations, though — a callable nested in the // result is registered before encoding finishes, and if encoding then // throws, the bytes never reach the engine, so it never decodes (and @@ try { const iv: baml_core.cffi.v1.IInboundValue = {}; setInboundValue(iv, value, ctx); const msg = InboundValue.create(iv); - bytes = Buffer.from(InboundValue.encode(msg).finish()); + const bytes = Buffer.from(InboundValue.encode(msg).finish()); + completeHostCall(callId, 0, bytes); } catch (err) { for (const k of ctx.registered) { try { releaseHostCallable(k); } catch { @@ } sendHostCallableError(callId, err); - return; } - completeHostCall(callId, 0, bytes); }🤖 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/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts` around lines 310 - 336, sendHostCallableResult currently calls completeHostCall(callId, 0, bytes) after the try/catch, so if completeHostCall throws the registered callables in ctx.registered leak; move the success-path native call inside the protected block or add a guarded finally: perform completeHostCall(callId, 0, bytes) inside the try after bytes are created and if completeHostCall throws run the same rollback loop over ctx.registered (calling releaseHostCallable) and call sendHostCallableError(callId, err) to report the failure; ensure ctx.registered cleanup is executed on any error and only call completeHostCall on a successful, fully protected path.
🤖 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 1836-1863: The helper currently returns
Err(EngineError::from(op_err)) when op_error_to_catchable_throw(...) is None,
but it must still settle a spawned child's future if vm_thread_settles_future()
is set; change the None branch to check thread.vm_thread_settles_future() and,
if Some(future_id), call the appropriate settling helper (e.g.,
settle_child_fatal(thread, future_id, EngineError::from(op_err)).await? or the
existing async settling API) and return Ok(ThreadOutcome::SettledChild); only
return Err(EngineError::from(op_err)) when no future_id is present. Use the
existing symbols vm_thread_settles_future, settle_child_fatal (or
settle_child_errored if that API expects a value) and
ThreadOutcome::SettledChild to locate and implement the fix.
---
Outside diff comments:
In `@baml_language/sdks/go/bridge_go/pkg/host_value.go`:
- Around line 356-380: The code registers host-callables in `registered` via
`goToInboundValueTracking` but never ensures those registrations are rolled back
if `completeHostCallSuccess` panics; add a defer immediately after
`goToInboundValueTracking` that checks if `registered` is non-nil and calls
`rollbackRegisteredHostValues(registered)` (and leaves `sendHostCallableError`
behavior unchanged), and after the successful `completeHostCallSuccess(callID,
resultBytes)` set `registered = nil` (or otherwise mark it cleared) so the defer
does not roll back on normal success; this ensures registrations are cleaned up
on panic while not undoing a real success.
---
Duplicate comments:
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts`:
- Around line 310-336: sendHostCallableResult currently calls
completeHostCall(callId, 0, bytes) after the try/catch, so if completeHostCall
throws the registered callables in ctx.registered leak; move the success-path
native call inside the protected block or add a guarded finally: perform
completeHostCall(callId, 0, bytes) inside the try after bytes are created and if
completeHostCall throws run the same rollback loop over ctx.registered (calling
releaseHostCallable) and call sendHostCallableError(callId, err) to report the
failure; ensure ctx.registered cleanup is executed on any error and only call
completeHostCall on a successful, fully protected path.
🪄 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: 19a64e1e-5f84-45dc-b0b5-09597ad79edf
⛔ Files ignored due to path filters (2)
baml_language/sdks/nodejs/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.map
📒 Files selected for processing (24)
baml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/host_value_callable.rsbaml_language/crates/bridge_cffi/src/ffi/host_value.rsbaml_language/crates/bridge_wasm/src/host_value.rsbaml_language/crates/bridge_wasm/src/lib.rsbaml_language/crates/sys_native/src/host_dispatch.rsbaml_language/crates/sys_native/src/host_impls.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/sdks/go/bridge_go/cffi/lib.gobaml_language/sdks/go/bridge_go/pkg/host_value.gobaml_language/sdks/go/bridge_go/pkg/host_value_internal_test.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/go/bridge_go/pkg/tests/host_callable_test.gobaml_language/sdks/nodejs/bridge_nodejs/package.jsonbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/host_value.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/python/rust/bridge_python/src/baml_core/baml_py/__init__.pyibaml_language/sdks/python/rust/bridge_python/src/host_value.rsbaml_language/sdks/python/src/baml_core/baml_py.pyibaml_language/sdks/python/src/baml_core/proto.pybaml_language/sdks/python/tests/test_host_callable.py
✅ Files skipped from review due to trivial changes (2)
- baml_language/sdks/python/src/baml_core/baml_py.pyi
- baml_language/sdks/python/rust/bridge_python/src/baml_core/baml_py/init.pyi
Contributor
|
Note Docstrings generation - IN PROGRESS Generating docstrings for this pull request |
meefs
pushed a commit
to meefs/baml
that referenced
this pull request
May 27, 2026
## Summary
Refutable pattern binding in condition position:
```baml
if let User { name } = u {
name
} else {
"no match"
}
```
* New `Expr::IfLet` AST node — distinct from `Expr::If`, no parse-time
desugaring (cleaner diagnostics + LSP).
* TIR: `infer_if_let_expr` / `check_if_let_expr` reuse match's pattern
infrastructure (`analyze_and_lower`, `finalize_pattern_lowering`,
exhaustiveness matrix) for binding registration and refutability
detection.
* MIR: `lower_if_let` via `lower_pattern_test` + `bind_pattern` (no
desugaring).
* `else if let` chaining handled by the existing else-branch dispatch.
* Scrutinee narrowing in then-branch (matched type) and else-branch
(residual) — same shape as `<expr> is <pat>`, plus pattern bindings.
* New warning `E0112 IrrefutablePatternInIfLet` when the matrix shows
the pattern covers every value — `else` would be dead.
* Formatter (`baml_fmt`) handles the new node with `IfLetExpr` +
`ElseExpr::IfLet`.
### Drive-by fix: `is`-destructure ambiguity
`if x is Class { ... }` used to greedily consume the then-block as a
destructure pattern body, breaking common code like `else if r is Empty
{ ... }`. Mirrored Rust's `NO_STRUCT_LITERAL` restriction — destructure
patterns are suppressed in `if`/`while` condition position, with parens
as an escape hatch (`if (x is Class { f })`).
## Test plan
24 new tests:
- [x] 4 parser unit tests (CST shape, ambiguity regression, parens
escape hatch)
- [x] 11 LSP fixture tests covering: basic narrowing & binding scope,
destructuring, irrefutable-pattern warning, `else if`/`else if let`
chaining, binding-doesn't-leak (3 cases), or-patterns, nested shadowing,
composite scrutinee, optional scrutinee, throws-analysis flow
- [x] 6 formatter tests (basic, no-else, chain, mixed plain/let,
destructure, or-pattern)
- [x] 11 end-to-end runtime tests in `bex_engine` (then/else dispatch,
destructure binding at runtime, or-pattern alternatives, nested
shadowing, composite scrutinee, optional scrutinee some/null, statement
form, chain dispatch + fall-through)
Full workspace test run: **5710 / 5714 pass**. The 4 failures are
pre-existing `sdk_test_python_pydantic2 *::pytest` cases blocked by a
stale precompiled `baml_py.abi3.so` missing the `register_host_callable`
symbol added by BoundaryML#3571 — unrelated to this PR.
## Follow-ups (not in this PR)
- `while let` — natural counterpart; same infrastructure would extend.
- Promote scrutinee narrowing through `&&` / `\|\|` chains for `if let`?
(Rust doesn't; we don't either today.)
- Quick-fix code action for the irrefutable warning (convert to plain
`let`).
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added `if let` conditional pattern matching with scoped then-branch
bindings, type narrowing, destructuring and or-pattern support, plus
`else if let` / mixed `else if` chains.
* Formatter and editor tooling now recognize and format `if let`;
completions treat `if let` as a value-position expression.
* **Diagnostics**
* New warning for irrefutable `if let` patterns (E0112) when the else
branch is unreachable.
* **Tests**
* Extensive unit/integration tests and fixtures covering scoping,
chaining, throws, destructuring, nullable scrutinees, and formatting.
<!-- review_stack_entry_start -->
[](https://app.coderabbit.ai/change-stack/BoundaryML/baml/pull/3579?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Jun 3, 2026
Merged
Merged
This was referenced Jun 10, 2026
Closed
](https://app.coderabbit.ai/change-stack/BoundaryML/baml/pull/3579?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem: we want to be able to pass lambdas/closures/function pointers into BAML from a host language in a type-safe way and let BAML call it as if it were a normal BAML function. BAML->host handles already exist in the language, allowing the host language to own opaque BAML values. This PR implements the opposite: a way to have opaque handles to host values in BAML.
Lifecycle
Summary by CodeRabbit