feat(bridges): support providing host callables as lambdas with optional args#3787
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (28)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (21)
📝 WalkthroughWalkthroughIntroduces a ChangesHost callable optional parameter dispatch
Sequence Diagram(s)sequenceDiagram
participant BAML as BAML VM (host_closure_call_sysop)
participant sysop as SysOp::BamlHostCallHostValue
participant native as sys_native / WASM
participant sdk as Go / Node.js / Python SDK
participant host as Host callback
rect rgba(70, 130, 180, 0.5)
note over BAML,sysop: Arg splitting (Rust VM)
BAML->>BAML: extract hc.params from HostClosure
BAML->>BAML: iterate user_args with params → positional[], optional{name→value}
BAML->>BAML: drop OMITTED_ARG optionals
BAML->>sysop: args[1] = [positional_ptr, optional_ptr]
end
rect rgba(60, 179, 113, 0.5)
note over sysop,native: Bridge encoding (sys_native / WASM)
sysop->>native: unpack [positional_array, optional_map]
native->>native: build_to_host_call → BamlToHostCall bytes
native->>native: fire_dispatch(args_bytes)
end
rect rgba(210, 105, 30, 0.5)
note over native,host: Bridge decoding (Go / Node.js / Python)
sdk->>sdk: unmarshal BamlToHostCall
sdk->>sdk: Node.js: reshapeHostArgs → [...positional, $opts?]
sdk->>sdk: Python: (positional_tuple, kwargs_dict)
sdk->>sdk: Go: rawArgs from GetArgs()
sdk->>host: callback invocation with shaped args
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57515579ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const head = rawArgs.slice(0, required); | ||
| const tail = rawArgs.slice(required); |
There was a problem hiding this comment.
Walk params instead of slicing required args
When a callable type declares an optional parameter before a required one, e.g. (x?: int, y: int) -> string, the engine still builds the host-call arg list in parameter order from the call plan, so a named call supplies raw args as [x, y]. This reshape counts all required params and slices that many values from the front, so x is delivered as the positional y argument and the real y is folded into $opts.x/lost. The parser/TIR allow this parameter order for function types, so the adapter needs to walk params and rawArgs together rather than assuming all required parameters are leading.
Useful? React with 👍 / 👎.
| if p.mode == baml_codegen_types::CodegenFunctionParamMode::Optional { | ||
| write!(sig, ", {pname}: {pty} = ...").unwrap(); | ||
| } else { | ||
| write!(sig, ", {pname}: {pty}").unwrap(); |
There was a problem hiding this comment.
Avoid default-before-required protocol signatures
For a valid callable type with an optional parameter before a required one, such as (x?: int, y: int) -> string, this emits def __call__(self, x: int = ..., y: int), which is invalid Python syntax because a non-default argument follows a default argument. That makes the generated .pyi unusable for any leaf exposing such a callback type; the Protocol signature needs to be rendered in a Python-valid order/convention instead of preserving the optional-before-required order with defaults.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
baml_language/sdks/python/rust/bridge_python/src/host_value.rs (2)
213-217:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
host_dispatch_callbackdoc to describe the newBamlHostDispatchenvelope format.The doc comment still describes
argsas a "protobuf-encodedBamlOutboundValue", but the implementation now expects aBamlHostDispatchenvelope (see lines 364-371 where it parsesBamlHostDispatchand extracts theargsfield).📝 Suggested documentation fix
/// Dispatch a BAML→host call into Python. /// -/// `args` is a protobuf-encoded `BamlOutboundValue` whose variant is a -/// `BamlValueList` (see `sys_native::host_impls::call_host_value` — -/// args are wrapped as `BexExternalValue::Array` before encoding). +/// `args` is a protobuf-encoded `BamlHostDispatch` envelope whose `args` +/// field contains a list-shaped `BamlOutboundValue` (see +/// `sys_native::host_impls::call_host_value`). #[expect(🤖 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 213 - 217, Update the documentation comment for the host_dispatch_callback function to accurately reflect the current implementation. The doc comment currently describes args as a protobuf-encoded BamlOutboundValue, but the implementation now expects a BamlHostDispatch envelope from which the args field is extracted during parsing. Replace the outdated description with an accurate one that references the BamlHostDispatch envelope format and explains that args is extracted from this envelope structure.
12-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate file header documentation to reflect the new envelope-based wire format.
Step 2 in the dispatch callback description still references decoding
BamlOutboundValuedirectly, but the implementation now first parses aBamlHostDispatchenvelope (lines 364-371) and then extracts itsargsfield before decoding.📝 Suggested documentation update
/// The dispatch callback: /// 1. Looks up the Python callable by `host_value_key`. -/// 2. Decodes the `BamlOutboundValue` args into Python values via -/// `baml_core.proto._decode_value_holder` (already a list shape). +/// 2. Parses the `BamlHostDispatch` envelope and decodes its `args` field +/// (a list-shaped `BamlOutboundValue`) into Python values via +/// `baml_core.proto.decode_value`. /// 3. Invokes the callable. If the return is a coroutine, runs it to🤖 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 12 - 16, The file header documentation at the top of the file describing the dispatch callback process is outdated. Update the documentation in the comment block (lines 12-16) to reflect the new envelope-based wire format. Instead of step 2 describing direct decoding of BamlOutboundValue, update it to first mention parsing the BamlHostDispatch envelope (as seen in the implementation around lines 364-371), then extracting the args field from that envelope, and finally decoding those args via baml_core.proto._decode_value_holder. This will align the documentation with the actual implementation flow.
🧹 Nitpick comments (1)
baml_language/crates/bridge_ctypes/src/value_encode.rs (1)
456-480: ⚡ Quick winAdd focused unit tests for the new dispatch/type helpers.
function_ty_to_protoandhost_dispatch_envelopedefine cross-bridge wire shape, but this file’s tests don’t directly lock their behavior yet. Please add unit tests that assert: (1) function params preserve name/optional/type mapping, (2) non-function inputs returnNone, and (3)host_dispatch_envelopesetsargs+callee_typeas expected.🤖 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_encode.rs` around lines 456 - 480, Add unit tests at the end of this file to lock the behavior of function_ty_to_proto and host_dispatch_envelope. Create tests that verify function_ty_to_proto correctly extracts and maps function parameters (preserving their name, optional, and type attributes) when given a RuntimeTy::Function, returns None for non-function types, and that host_dispatch_envelope correctly populates both the args field with the input BamlOutboundValue and the callee_type field by calling function_ty_to_proto on the provided RuntimeTy.
🤖 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/nodejs/bridge_nodejs/typescript_src/proto.ts`:
- Around line 469-477: The functions `decodeHostDispatch` and `reshapeHostArgs`
are silently coercing or dropping arguments instead of validating dispatch
arity. In `decodeHostDispatch`, instead of treating a missing or falsy `args`
field as an empty list, add validation to ensure the decoded arguments match the
expected arity for the callable. In `reshapeHostArgs`, instead of silently
dropping overflow optional-tail values when the rawArgs length exceeds declared
optional parameters, add validation that throws an error when the argument count
does not match the expected arity. This will surface contract violations rather
than silently mis-mapping callback arguments.
In `@baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/leaf.rs`:
- Around line 171-173: The FQN leaf segment obtained from the fqn_leaf function
call (which assigns to the owner variable) and the arg.name references contain
unsanitized characters including potential $ companion suffixes that are invalid
in Python identifiers. Apply sanitization to the owner variable after the
fqn_leaf call at line 171 and to arg.name when it is used in the format string
at line 173, as well as to parameter names at lines 187 and 1872 where callback
Protocol bases are being generated. Ensure all identifiers used in the Protocol
class names in the .pyi output contain only valid Python identifier characters.
---
Outside diff comments:
In `@baml_language/sdks/python/rust/bridge_python/src/host_value.rs`:
- Around line 213-217: Update the documentation comment for the
host_dispatch_callback function to accurately reflect the current
implementation. The doc comment currently describes args as a protobuf-encoded
BamlOutboundValue, but the implementation now expects a BamlHostDispatch
envelope from which the args field is extracted during parsing. Replace the
outdated description with an accurate one that references the BamlHostDispatch
envelope format and explains that args is extracted from this envelope
structure.
- Around line 12-16: The file header documentation at the top of the file
describing the dispatch callback process is outdated. Update the documentation
in the comment block (lines 12-16) to reflect the new envelope-based wire
format. Instead of step 2 describing direct decoding of BamlOutboundValue,
update it to first mention parsing the BamlHostDispatch envelope (as seen in the
implementation around lines 364-371), then extracting the args field from that
envelope, and finally decoding those args via
baml_core.proto._decode_value_holder. This will align the documentation with the
actual implementation flow.
---
Nitpick comments:
In `@baml_language/crates/bridge_ctypes/src/value_encode.rs`:
- Around line 456-480: Add unit tests at the end of this file to lock the
behavior of function_ty_to_proto and host_dispatch_envelope. Create tests that
verify function_ty_to_proto correctly extracts and maps function parameters
(preserving their name, optional, and type attributes) when given a
RuntimeTy::Function, returns None for non-function types, and that
host_dispatch_envelope correctly populates both the args field with the input
BamlOutboundValue and the callee_type field by calling function_ty_to_proto on
the provided RuntimeTy.
🪄 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: 1a0f9d4d-951e-473e-92f2-3cda66494ef1
⛔ Files ignored due to path filters (12)
baml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/optional_parameter_defaults/baml_tests__broken_syntax__optional_parameter_defaults__01_lexer__callback_param_defaults.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/optional_parameter_defaults/baml_tests__broken_syntax__optional_parameter_defaults__02_parser__callback_param_defaults.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/optional_parameter_defaults/baml_tests__broken_syntax__optional_parameter_defaults__05_diagnostics.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/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_outbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.jsis excluded by!**/dist/**baml_language/sdks/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
baml_language/crates/baml_builtins2/baml_std/baml/ns_host/host.bamlbaml_language/crates/baml_tests/projects/broken_syntax/optional_parameter_defaults/callback_param_defaults.bamlbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/tests/host_value_callable.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm_types/src/types.rsbaml_language/crates/bridge_ctypes/src/lib.rsbaml_language/crates/bridge_ctypes/src/value_encode.rsbaml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_outbound.protobaml_language/crates/bridge_wasm/src/host_value.rsbaml_language/crates/sys_native/src/host_impls.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_host_callables.pybaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/host_callables.test.tsbaml_language/sdk_tests/fixtures/function_calls/baml_src/ns_host_callable_tests/main.bamlbaml_language/sdks/go/bridge_go/pkg/host_value.gobaml_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/nodejs/sdkgen_typescript_node/src/leaf.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/translate_ty.rsbaml_language/sdks/python/rust/bridge_python/src/host_value.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/leaf.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/translate_ty.rsbaml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.pybaml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.pyi
5751557 to
4c94c56
Compare
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
No description provided. |
4c94c56 to
ac13f46
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 similar comment
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
ac13f46 to
66c1a94
Compare
66c1a94 to
0f52745
Compare
0f52745 to
9b4c96b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/bex_engine/tests/host_value_callable.rs`:
- Around line 218-222: The code in the host_value_callable.rs test uses
filter_map on to_host_args.args which silently drops any args where the value
field is None, masking potential wire-format regressions. Replace the filter_map
call with map combined with expect() or unwrap() to fail fast when encountering
a decoded arg with a missing value field, ensuring that malformed arguments are
caught immediately rather than being silently filtered out.
🪄 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: 6523747e-905f-445a-a8b5-d236a3b5d2d0
⛔ Files ignored due to path filters (12)
baml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/optional_parameter_defaults/baml_tests__broken_syntax__optional_parameter_defaults__01_lexer__callback_param_defaults.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/optional_parameter_defaults/baml_tests__broken_syntax__optional_parameter_defaults__02_parser__callback_param_defaults.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/optional_parameter_defaults/baml_tests__broken_syntax__optional_parameter_defaults__05_diagnostics.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/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_outbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.jsis excluded by!**/dist/**baml_language/sdks/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
baml_language/crates/baml_builtins2/baml_std/baml/ns_host/host.bamlbaml_language/crates/baml_tests/projects/broken_syntax/optional_parameter_defaults/callback_param_defaults.bamlbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/tests/host_value_callable.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm_types/src/types.rsbaml_language/crates/bridge_ctypes/src/lib.rsbaml_language/crates/bridge_ctypes/src/value_encode.rsbaml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_outbound.protobaml_language/crates/bridge_wasm/src/host_value.rsbaml_language/crates/bridge_wasm/tests/host_callable.rsbaml_language/crates/sys_native/src/host_impls.rsbaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_host_callables.pybaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/host_callables.test.tsbaml_language/sdk_tests/fixtures/function_calls/baml_src/ns_host_callable_tests/main.bamlbaml_language/sdks/go/bridge_go/pkg/host_value.gobaml_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/nodejs/sdkgen_typescript_node/src/leaf.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/translate_ty.rsbaml_language/sdks/python/rust/bridge_python/src/host_value.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/leaf.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/translate_ty.rsbaml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.pybaml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.pyi
✅ Files skipped from review due to trivial changes (4)
- baml_language/crates/baml_builtins2/baml_std/baml/ns_host/host.baml
- 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
- baml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.py
🚧 Files skipped from review as they are similar to previous changes (19)
- baml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.pyi
- baml_language/sdk_tests/fixtures/function_calls/baml_src/ns_host_callable_tests/main.baml
- baml_language/crates/bridge_ctypes/src/lib.rs
- baml_language/sdk_tests/crates/typescript_node/function_calls/customizable/host_callables.test.ts
- baml_language/crates/baml_tests/projects/broken_syntax/optional_parameter_defaults/callback_param_defaults.baml
- baml_language/crates/bex_vm_types/src/types.rs
- baml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_outbound.proto
- baml_language/sdks/go/bridge_go/pkg/host_value.go
- baml_language/sdks/python/rust/bridge_python/src/host_value.rs
- baml_language/crates/bridge_wasm/tests/host_callable.rs
- baml_language/crates/sys_native/src/host_impls.rs
- baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/translate_ty.rs
- baml_language/crates/bridge_wasm/src/host_value.rs
- baml_language/sdks/nodejs/sdkgen_typescript_node/src/leaf.rs
- baml_language/crates/bex_vm/src/vm.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts
- baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/leaf.rs
- baml_language/crates/bex_engine/src/conversion.rs
- baml_language/sdks/nodejs/sdkgen_typescript_node/src/translate_ty.rs
9b4c96b to
fc3b3a4
Compare
fc3b3a4 to
a93ed3c
Compare
Summary by CodeRabbit
Release Notes
New Features
$opts?: { ... }object.Bug Fixes
Documentation
Tests