Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Go as Go Client
participant CFFI as CFFI Bindings<br/>(lib.go)
participant Rust as Rust FFI<br/>(bridge_go lib)
participant Engine as BAML Engine
participant CB as Go Callback<br/>(callbacks.go)
Go->>CFFI: CallFunction(name, args, id)
CFFI->>Rust: call_function(name, args, id)
activate Rust
Rust->>Engine: runtime.call_function(name, kwargs)
activate Engine
Note over Engine: Async execution
Engine-->>Rust: Result or Error
deactivate Engine
alt Success Path
Rust->>CB: result_callback(id, is_done, data)
CB->>CB: Decode & send to channel
else Error Path
Rust->>CB: error_callback(id, is_done, error_msg)
CB->>CB: Parse error & send to channel
end
deactivate Rust
CB-->>Go: Channel receives ResultCallback
Go->>Go: Unblock CallFunction waiter
Go-->>Go: Return result or error to caller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a59b9eee6
ℹ️ 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".
Comment on lines
+55
to
+57
| case <-ctx.Done(): | ||
| deleteCallback(callbackID) | ||
| return nil, ctx.Err() |
There was a problem hiding this comment.
When ctx.Done() fires, this branch only removes the local callback entry and returns, but it never signals the Rust engine to cancel the in-flight call. That means timed-out/canceled requests keep running in the background (and can still consume LLM/runtime resources) even though the caller already received context.Canceled/DeadlineExceeded.
Useful? React with 👍 / 👎.
Comment on lines
+144
to
+145
| default: | ||
| return nil, fmt.Errorf("unsupported outbound value type: %T", val.Value) |
There was a problem hiding this comment.
outboundToGo falls through to an error for unhandled variants, and there is no case for BamlOutboundValue_HandleValue (nor checked/streaming wrappers). Rust can emit handle_value for opaque return values via in-process handle-table encoding, so valid function results are surfaced as decode failures instead of usable Go values.
Useful? React with 👍 / 👎.
Comment on lines
+69
to
+70
| default: | ||
| return nil, fmt.Errorf("unsupported type: %T", v) |
There was a problem hiding this comment.
The argument encoder rejects pkg.BamlHandle and returns unsupported type, so handles cannot be passed back into subsequent function calls. This breaks handle round-tripping even though the bridge exposes BamlHandle and the inbound protobuf schema includes a handle field.
Useful? React with 👍 / 👎.
![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: 3
🧹 Nitpick comments (18)
baml_language/crates/bridge_go/pkg/tests/handle_test.go (1)
9-23: Consider adding one positive-path handle lifecycle test.Current coverage validates invalid key behavior well; adding a runtime-backed valid handle case would also protect the successful
Clone/Releasepath from regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/pkg/tests/handle_test.go` around lines 9 - 23, Add a positive-path test that exercises the successful handle lifecycle: in the same TestCloneAndReleaseHandle (or a new TestCloneAndReleaseHandle_Success) obtain a runtime-backed valid pkg.BamlHandle (e.g., call the package factory or function that returns a live handle instead of constructing Key:0), assert the returned handle.Key is non-zero, call Clone() on that handle and assert cloned.Key equals the original and is non-zero, then call Release() on both original and clone and ensure no panics or errors; reference the pkg.BamlHandle type and its Clone and Release methods when adding the assertions.baml_language/crates/bridge_go/src/handle.rs (1)
3-11: Add Rust unit tests to lock these FFI contracts.
clone_handlereturning0on miss andrelease_handlebeing safe on invalid keys are ABI-visible behaviors; please codify them with local#[cfg(test)]tests in this file.Proposed test scaffold
+#[cfg(test)] +mod tests { + use super::{clone_handle, release_handle}; + + #[test] + fn clone_invalid_handle_returns_zero() { + assert_eq!(clone_handle(0, 0), 0); + } + + #[test] + fn release_invalid_handle_is_noop() { + release_handle(0, 0); + } +}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 the current code and only fix it if needed. In `@baml_language/crates/bridge_go/src/handle.rs` around lines 3 - 11, Add Rust unit tests in this file under a #[cfg(test)] mod to lock the FFI contracts: write one test that calls clone_handle with a known-missing key and asserts it returns 0 (use HANDLE_TABLE keys that are not inserted or a very large u64), and write another test that calls release_handle with an invalid/nonexistent key and verifies it does not panic (use std::panic::catch_unwind and assert the call completes). Reference the exported functions clone_handle and release_handle and the HANDLE_TABLE symbol in the tests so the ABI behaviors are codified.baml_language/crates/bridge_go/go.mod (1)
5-5: Update protobuf version to align with repository baseline.
bridge_go/go.modpinsgoogle.golang.org/protobuftov1.33.0, while all other Go modules in the repository usev1.36.6. Both versions are compatible (same runtime API), so update tov1.36.6to maintain consistency across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/go.mod` at line 5, The protobuf dependency is pinned to google.golang.org/protobuf v1.33.0; update that require line to use v1.36.6 so it matches the repository baseline. Locate the require entry for google.golang.org/protobuf (currently v1.33.0) in go.mod and change the version to v1.36.6, then run go mod tidy to refresh go.sum and ensure consistency.baml_language/crates/bridge_go/src/errors.rs (1)
3-60: Consider adding unit tests for error stringification.This module converts
BridgeErrorvariants to categorized strings that the Go side parses viaParseBamlError. Unit tests would help ensure the prefix format (e.g.,"BamlError: BamlInvalidArgumentError:") remains stable across refactors.💡 Example test structure
#[cfg(test)] mod tests { use super::*; #[test] fn test_not_initialized_error() { let err = BridgeError::NotInitialized; let s = bridge_error_to_string(&err); assert!(s.starts_with("BamlError: BamlInvalidArgumentError:")); } #[test] fn test_function_not_found_error() { let err = BridgeError::FunctionNotFound { name: "foo".into() }; let s = bridge_error_to_string(&err); assert!(s.contains("Function not found: foo")); } }As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/src/errors.rs` around lines 3 - 60, Add unit tests in errors.rs to assert stable string prefixes and contents produced by bridge_error_to_string and runtime_error_to_string: create tests for BridgeError::NotInitialized, BridgeError::FunctionNotFound { name }, BridgeError::MissingArgument { function, parameter }, BridgeError::Internal(msg), BridgeError::Ctypes, and a RuntimeError case (e.g., RuntimeError::InvalidArgument and EngineError::Cancelled / EngineError::FunctionNotFound) to ensure strings start with the expected category tokens like "BamlError: BamlInvalidArgumentError:" or "BamlError: BamlClientError:" and contain the specific details; call bridge_error_to_string and runtime_error_to_string directly and use assertions like starts_with and contains to lock the format.baml_language/crates/bridge_go/src/runtime.rs (1)
30-32: Clarify the sentinel pointer usage.
std::ptr::dangling::<libc::c_void>()returns a well-aligned non-null pointer that doesn't point to valid memory. This works as a sentinel since the runtime is global, but a brief doc comment or using a named constant would improve clarity for future maintainers.📝 Suggested documentation
+ // Return a non-null sentinel pointer. The actual runtime is stored globally + // in bridge_cffi::engine, so this pointer is never dereferenced. Ok(std::ptr::dangling::<libc::c_void>())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/src/runtime.rs` around lines 30 - 32, The use of std::ptr::dangling::<libc::c_void>() as a sentinel pointer in runtime.rs is unclear to future readers; replace the inline call with a clearly named sentinel (e.g., RUNTIME_SENTINEL) or add a short doc comment on the function that returns it explaining that it is a well-aligned, non-null sentinel used because the runtime is global; update the return site where std::ptr::dangling::<libc::c_void>() is currently used to reference the named constant or add the doc comment above the function so maintainers see the intent immediately.baml_language/crates/bridge_go/pkg/tests/call_function_test.go (2)
64-77: Shared mutable state without synchronization.
testRTis a package-level variable accessed by multiple test functions. While Go's test runner executes tests sequentially by default, ift.Parallel()is ever added, this will race. Consider usingsync.Oncefor thread-safe lazy initialization.💡 Optional: Use sync.Once for safety
+var testRTOnce sync.Once +var testRTErr error + func getTestRuntime(t *testing.T) *pkg.BamlRuntime { t.Helper() - if testRT != nil { - return testRT - } - rt, err := pkg.NewRuntime(".", map[string]string{"main.baml": callFunctionBamlSource}) - if err != nil { - t.Fatalf("NewRuntime failed: %v", err) - } - testRT = rt + testRTOnce.Do(func() { + testRT, testRTErr = pkg.NewRuntime(".", map[string]string{"main.baml": callFunctionBamlSource}) + }) + if testRTErr != nil { + t.Fatalf("NewRuntime failed: %v", testRTErr) + } return testRT }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/pkg/tests/call_function_test.go` around lines 64 - 77, The package-level variable testRT is lazily initialized in getTestRuntime and can race if tests run in parallel; replace the ad-hoc nil check with a sync.Once-based initialization (declare a once variable and use once.Do to call the pkg.NewRuntime creation path) so getTestRuntime uses once.Do to create testRT (and capture/handle any error inside the once body or propagate it safely), referencing testRT, getTestRuntime, and pkg.NewRuntime to locate where to apply the change.
135-142: Unchecked type assertions may panic.Lines 135 and 139 use direct type assertions (
result.(map[string]any)) without checking theokboolean, unlike other tests in this file (e.g., lines 118-121). A failed assertion will panic rather than produce a clear test failure.♻️ Use checked type assertions for consistency
- m := result.(map[string]any) + m, ok := result.(map[string]any) + if !ok { + t.Fatalf("expected map, got %T", result) + } if m["name"] != "Bob" { t.Fatalf("expected name=Bob, got %v", m["name"]) } - addr := m["address"].(map[string]any) + addr, ok := m["address"].(map[string]any) + if !ok { + t.Fatalf("expected address to be map, got %T", m["address"]) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/pkg/tests/call_function_test.go` around lines 135 - 142, Replace the unchecked type assertions that can panic—change result.(map[string]any) to a comma-ok checked assertion (e.g., m, ok := result.(map[string]any); if !ok { t.Fatalf("expected result to be map[string]any, got %T", result) }) and likewise change addr := m["address"].(map[string]any) to a checked form (addr, ok := m["address"].(map[string]any); if !ok { t.Fatalf("expected m[\"address\"] to be map[string]any, got %T", m["address"]) }), and also validate the types of m["name"] and addr["zip"] before comparing values to avoid panics.baml_language/crates/bridge_go/cffi/lib.go (4)
216-219: Slice-to-pointer conversion is safe but fragile.Taking
&encodedArgs[0]is safe here becauselen(encodedArgs) > 0is checked first. This is a common Go/cgo pattern. Consider adding a comment for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/cffi/lib.go` around lines 216 - 219, The slice-to-pointer conversion for encodedArgs into cArgs is safe due to the preceding len(encodedArgs) > 0 check but is fragile; add an inline comment next to the conversion (where encodedArgs and cArgs are used in lib.go) explaining that the len check guarantees &encodedArgs[0] is valid for cgo, e.g., "safe: len checked above" and optionally mention why we don't use nil for empty slices, so future maintainers don't remove the check or refactor incorrectly.
183-194:CreateBamlRuntimereturnsnilerror message on failure.When
wrapCreateBamlRuntimereturnsnil, the error message is generic:"create_baml_runtime failed (returned null)". The Rust side may have set a more descriptive error via a different mechanism. Consider whether the Rust FFI should return an error buffer instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/cffi/lib.go` around lines 183 - 194, CreateBamlRuntime currently reports a generic error when C.wrapCreateBamlRuntime returns nil; update the FFI to surface more descriptive Rust-side errors by either (a) modifying the Rust API to return an error buffer/string (and exposing a new C wrapper like wrapGetLastError or returning a struct with ptr+err), or (b) adding an exported function (e.g., wrap_baml_last_error) that retrieves the last error message set by Rust; then change CreateBamlRuntime to call that function when ptr == nil and include the returned error text in the fmt.Errorf message instead of the generic "create_baml_runtime failed (returned null)".
142-167:FindLibraryassumes standard cargo output paths.The function only checks
target/debugandtarget/release. Custom cargo profiles orCARGO_TARGET_DIRoverrides will cause the library to not be found. This is acceptable for development/testing but should be documented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/cffi/lib.go` around lines 142 - 167, FindLibrary currently only checks target/debug and target/release which breaks when CARGO_TARGET_DIR or custom cargo profiles are used; update FindLibrary to also consult the CARGO_TARGET_DIR environment variable (if set) and add candidates under that directory (both debug/release or the profile dir), and additionally walk upward from crateDir to discover any "target" directories (e.g., search parent dirs for a "target" folder and add debug/release candidates found there); ensure the returned error includes the expanded absolute paths that were attempted for easier debugging. Use the function name FindLibrary and variables candidates/crateDir when implementing these checks.
129-139:dlsymreturningnilmay not indicate an error.Per POSIX,
dlsymcan legitimately returnNULLif the symbol's value isNULL. The check on line 134-137 would incorrectly treat this as an error. However, for function pointers in a Rust cdylib, this is practically impossible, so this is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/cffi/lib.go` around lines 129 - 139, The current resolveSymbol treats any nil from C.dlsym as an error; instead, call C.dlerror() after dlsym and only treat it as an error if dlerror returns non-NULL. In resolveSymbol, keep the initial C.dlerror() to clear errors, call sym := C.dlsym(handle, cName), then check errPtr := C.dlerror(); if errPtr != nil convert it (C.GoString) and return an error, otherwise return sym, nil (allowing a legitimate NULL symbol value without error).baml_language/crates/bridge_go/pkg/callbacks.go (3)
44-52: Silent panic recovery may obscure bugs.
safeSendandsafeClosesilently swallow panics without any logging. While this prevents crashes when sending to a closed channel, it could mask programming errors during development. Consider adding debug logging.💡 Optional: Add debug logging
func safeSend(ch chan ResultCallback, res ResultCallback) { - defer func() { recover() }() + defer func() { + if r := recover(); r != nil { + // Channel was closed; callback already handled + } + }() ch <- res }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/pkg/callbacks.go` around lines 44 - 52, safeSend and safeClose currently swallow panics silently via recover(), which can hide bugs; update both functions (safeSend, safeClose) to check the recovered value and, when non-nil, emit a debug-level log message including the recovered value and a short context string (e.g., "panic sending to channel" / "panic closing channel") so developers can see the issue in logs; use the existing project logger if available or the standard log package, and keep the recover to avoid crashes while surfacing the error for debugging.
76-90: UnusedisDoneparameter inerror_callback.The
isDoneparameter is declared but never used—the function always closes the channel regardless. This is fine if intentional (errors are always terminal), but the parameter could be removed from the Go signature if Rust always passes1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/pkg/callbacks.go` around lines 76 - 90, The exported function error_callback currently ignores the isDone parameter; update it so the handler respects isDone: read isDone (e.g., if isDone != 0) then send the error and close the callback channel and deleteCallback(uint32(id)), otherwise only send the error on cb.channel without closing or deleting so non-terminal errors are handled; if instead Rust truly always passes terminal errors, remove the isDone parameter from error_callback's signature and adjust any corresponding C/Rust callers to stop passing it. Ensure you reference the dynamicCallbacks lookup, cb.channel usage, safeClose(cb.channel) and deleteCallback(uint32(id)) when making the change.
29-36: Potential ID wraparound after ~4 billion calls.The
nextCallbackIDcounter will wrap around to 0 aftermath.MaxUint32calls. If old callbacks are still pending when this happens, ID collisions could cause incorrect routing. This is unlikely in practice but worth documenting or handling defensively.💡 Optional: Add collision check
func createUniqueID() (uint32, chan ResultCallback) { id := nextCallbackID.Add(1) + // Note: ID 0 is reserved/skipped after wraparound + if id == 0 { + id = nextCallbackID.Add(1) + } ch := make(chan ResultCallback, 64) callbackMutex.Lock() + if _, exists := dynamicCallbacks[id]; exists { + // Extremely unlikely collision after wraparound + callbackMutex.Unlock() + return createUniqueID() // retry + } dynamicCallbacks[id] = callbackData{channel: ch} callbackMutex.Unlock() return id, ch }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/pkg/callbacks.go` around lines 29 - 36, The createUniqueID function can produce colliding IDs after nextCallbackID wraps; change it to loop until a free id is found by repeatedly incrementing nextCallbackID and checking dynamicCallbacks under callbackMutex before inserting so you never overwrite an existing entry. Specifically, inside createUniqueID use a loop that calls nextCallbackID.Add(1), acquires callbackMutex, checks if dynamicCallbacks contains that id, and only then creates/assigns the channel and returns the id and channel (ensure the channel is created only when a free id is confirmed); reference createUniqueID, nextCallbackID, dynamicCallbacks, and callbackMutex when making the change.baml_language/crates/bridge_go/pkg/runtime.go (2)
49-58: Context cancellation only abandons the Go-side wait, not the Rust task.When
ctx.Done()fires, the callback is deleted andctx.Err()is returned, but the spawned Rust task continues executing. This is acceptable for short-lived operations, but for long-running LLM calls, the Rust side will complete and attempt to send to a deleted callback (which is handled gracefully). Consider documenting this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/pkg/runtime.go` around lines 49 - 58, The select that waits on ch vs ctx.Done() (using ch, callbackID, deleteCallback, ctx.Done(), result.Error/result.Data) only aborts the Go-side wait and deletes the callback; the spawned Rust task continues and may later attempt to send to the deleted callback (which the Rust side already handles). Update the function's documentation/comments to explicitly state this behavior and tradeoff (Go cancels local wait and removes callback but does not cancel the Rust task), and mention that if true cancellation is required the Rust side must be taught to accept/observe cancellation tokens or an explicit cancellation API should be added; reference ch, callbackID and deleteCallback in the doc so callers know why the Rust task continues after ctx cancellation.
13-15:ptrfield is stored but never used.The
BamlRuntime.ptrfield is assigned duringNewRuntimebut never referenced inCallFunctionor elsewhere. Per the AI summary, the runtime is global, so this pointer is effectively a sentinel. Consider documenting this or removing the field if it serves no purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/pkg/runtime.go` around lines 13 - 15, The BamlRuntime.ptr field is unused—remove the sentinel field and any assignments to it in NewRuntime, and update related code (BamlRuntime struct definition and NewRuntime) so callers and CallFunction use the remaining fields/methods without relying on ptr; alternatively, if you want to preserve a sentinel, add a clear comment above the ptr field explaining its purpose and ensure NewRuntime/CallFunction reference it (or a no-op cast) so it's intentionally documented rather than left unused.baml_language/crates/bridge_go/src/callbacks.rs (1)
11-18: Silent failure on re-registration could mask initialization bugs.
OnceCell::set()silently discards the error when callbacks are already registered. While idempotent behavior is intentional, a debug log or assert in debug builds could help catch unintended double-initialization.💡 Optional: Log on re-registration attempt
pub extern "C" fn register_callbacks( result_callback_fn: CallbackFn, error_callback_fn: CallbackFn, ) { - let _ = RESULT_CALLBACK_FN.set(result_callback_fn); - let _ = ERROR_CALLBACK_FN.set(error_callback_fn); + if RESULT_CALLBACK_FN.set(result_callback_fn).is_err() { + #[cfg(debug_assertions)] + eprintln!("bridge_go: result callback already registered"); + } + if ERROR_CALLBACK_FN.set(error_callback_fn).is_err() { + #[cfg(debug_assertions)] + eprintln!("bridge_go: error callback already registered"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/src/callbacks.rs` around lines 11 - 18, The register_callbacks function currently ignores OnceCell::set() errors which silently hides double-registration; update register_callbacks to check the Result returned by RESULT_CALLBACK_FN.set(...) and ERROR_CALLBACK_FN.set(...), and when set returns Err (meaning already initialized) either emit a debug-only assertion (e.g., debug_assert!(false, "register_callbacks called twice for RESULT_CALLBACK_FN")) or write a debug log (e.g., eprintln! or crate logger) that includes the symbol name and the existing/attempted state so double-initialization is visible during development; keep the public API unchanged but ensure the check references register_callbacks, RESULT_CALLBACK_FN, ERROR_CALLBACK_FN and OnceCell::set to locate the code.baml_language/crates/bridge_go/src/functions.rs (1)
1-98: Consider adding unit tests for this module.As per coding guidelines, prefer writing Rust unit tests where possible. This module has testable logic (argument decoding, error encoding) that could benefit from unit tests.
Based on learnings: "Prefer writing Rust unit tests over integration tests where possible"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_go/src/functions.rs` around lines 1 - 98, Add Rust unit tests for this module by creating a #[cfg(test)] mod tests that exercises the pure/testable parts: 1) test call_function_inner returns Err(BridgeError::NullFunctionName) when function_name is null, 2) test decoding of CallFunctionArgs from a known buffer and conversion via kwargs_to_bex_values to ensure argument decoding succeeds, and 3) test encode_success() and encode_error() produce CallAck protobufs with expected response variants (decode the Buffer back to CallAck and assert the response). Use the existing symbols call_function_inner, CallFunctionArgs, kwargs_to_bex_values, encode_success, and encode_error and mock or construct minimal inputs (e.g., empty/default CallFunctionArgs) so tests do not require the async runtime or external services. Ensure tests live in the same file under a tests module and assert expected Result or decoded protobuf values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@baml_language/crates/bridge_go/build.sh`:
- Around line 11-15: The paired export+command-substitution in build.sh masks
command failures (SC2155); split the assignment and export so failures surface:
assign the dirname result to a local variable (e.g., protoc_dir or mise_dir)
using command substitution (for both the PROTOC_GEN_GO_PATH branch and the mise
branch that calls `mise which protoc-gen-go`), check the command succeeded if
needed, then export PATH="$protoc_dir:$PATH" (or export PATH="$mise_dir:$PATH");
update the two blocks around the export PATH lines to use these separate
assignment-then-export steps so non-zero exits are not masked.
In `@baml_language/crates/bridge_go/pkg/tests/call_function_test.go`:
- Around line 107-109: The failure message is inconsistent with the asserted
value: the test asserts that result == "red" but t.Fatalf says "expected 'Red'";
update the t.Fatalf call that references result to match the asserted lowercase
value (e.g., "expected 'red', got %v") so the error message accurately reflects
the expectation; locate the t.Fatalf call in call_function_test.go that checks
the variable result and adjust its message accordingly.
In `@baml_language/crates/bridge_go/src/runtime.rs`:
- Around line 16-22: Add explicit null-pointer checks for the incoming C
pointers before calling CStr::from_ptr: inside the ffi_safe_ptr closure, test if
root_path.is_null() and src_files_json.is_null() and return an Err with a
descriptive message (e.g., format!("Null pointer for root_path") / format!("Null
pointer for src_files_json")) instead of calling unsafe CStr::from_ptr; then
proceed to call unsafe CStr::from_ptr(...) only after each pointer is confirmed
non-null. Ensure you reference the existing symbols ffi_safe_ptr, root_path,
src_files_json and CStr::from_ptr when making the changes so the early-return
error integrates with the surrounding Result flow.
---
Nitpick comments:
In `@baml_language/crates/bridge_go/cffi/lib.go`:
- Around line 216-219: The slice-to-pointer conversion for encodedArgs into
cArgs is safe due to the preceding len(encodedArgs) > 0 check but is fragile;
add an inline comment next to the conversion (where encodedArgs and cArgs are
used in lib.go) explaining that the len check guarantees &encodedArgs[0] is
valid for cgo, e.g., "safe: len checked above" and optionally mention why we
don't use nil for empty slices, so future maintainers don't remove the check or
refactor incorrectly.
- Around line 183-194: CreateBamlRuntime currently reports a generic error when
C.wrapCreateBamlRuntime returns nil; update the FFI to surface more descriptive
Rust-side errors by either (a) modifying the Rust API to return an error
buffer/string (and exposing a new C wrapper like wrapGetLastError or returning a
struct with ptr+err), or (b) adding an exported function (e.g.,
wrap_baml_last_error) that retrieves the last error message set by Rust; then
change CreateBamlRuntime to call that function when ptr == nil and include the
returned error text in the fmt.Errorf message instead of the generic
"create_baml_runtime failed (returned null)".
- Around line 142-167: FindLibrary currently only checks target/debug and
target/release which breaks when CARGO_TARGET_DIR or custom cargo profiles are
used; update FindLibrary to also consult the CARGO_TARGET_DIR environment
variable (if set) and add candidates under that directory (both debug/release or
the profile dir), and additionally walk upward from crateDir to discover any
"target" directories (e.g., search parent dirs for a "target" folder and add
debug/release candidates found there); ensure the returned error includes the
expanded absolute paths that were attempted for easier debugging. Use the
function name FindLibrary and variables candidates/crateDir when implementing
these checks.
- Around line 129-139: The current resolveSymbol treats any nil from C.dlsym as
an error; instead, call C.dlerror() after dlsym and only treat it as an error if
dlerror returns non-NULL. In resolveSymbol, keep the initial C.dlerror() to
clear errors, call sym := C.dlsym(handle, cName), then check errPtr :=
C.dlerror(); if errPtr != nil convert it (C.GoString) and return an error,
otherwise return sym, nil (allowing a legitimate NULL symbol value without
error).
In `@baml_language/crates/bridge_go/go.mod`:
- Line 5: The protobuf dependency is pinned to google.golang.org/protobuf
v1.33.0; update that require line to use v1.36.6 so it matches the repository
baseline. Locate the require entry for google.golang.org/protobuf (currently
v1.33.0) in go.mod and change the version to v1.36.6, then run go mod tidy to
refresh go.sum and ensure consistency.
In `@baml_language/crates/bridge_go/pkg/callbacks.go`:
- Around line 44-52: safeSend and safeClose currently swallow panics silently
via recover(), which can hide bugs; update both functions (safeSend, safeClose)
to check the recovered value and, when non-nil, emit a debug-level log message
including the recovered value and a short context string (e.g., "panic sending
to channel" / "panic closing channel") so developers can see the issue in logs;
use the existing project logger if available or the standard log package, and
keep the recover to avoid crashes while surfacing the error for debugging.
- Around line 76-90: The exported function error_callback currently ignores the
isDone parameter; update it so the handler respects isDone: read isDone (e.g.,
if isDone != 0) then send the error and close the callback channel and
deleteCallback(uint32(id)), otherwise only send the error on cb.channel without
closing or deleting so non-terminal errors are handled; if instead Rust truly
always passes terminal errors, remove the isDone parameter from error_callback's
signature and adjust any corresponding C/Rust callers to stop passing it. Ensure
you reference the dynamicCallbacks lookup, cb.channel usage,
safeClose(cb.channel) and deleteCallback(uint32(id)) when making the change.
- Around line 29-36: The createUniqueID function can produce colliding IDs after
nextCallbackID wraps; change it to loop until a free id is found by repeatedly
incrementing nextCallbackID and checking dynamicCallbacks under callbackMutex
before inserting so you never overwrite an existing entry. Specifically, inside
createUniqueID use a loop that calls nextCallbackID.Add(1), acquires
callbackMutex, checks if dynamicCallbacks contains that id, and only then
creates/assigns the channel and returns the id and channel (ensure the channel
is created only when a free id is confirmed); reference createUniqueID,
nextCallbackID, dynamicCallbacks, and callbackMutex when making the change.
In `@baml_language/crates/bridge_go/pkg/runtime.go`:
- Around line 49-58: The select that waits on ch vs ctx.Done() (using ch,
callbackID, deleteCallback, ctx.Done(), result.Error/result.Data) only aborts
the Go-side wait and deletes the callback; the spawned Rust task continues and
may later attempt to send to the deleted callback (which the Rust side already
handles). Update the function's documentation/comments to explicitly state this
behavior and tradeoff (Go cancels local wait and removes callback but does not
cancel the Rust task), and mention that if true cancellation is required the
Rust side must be taught to accept/observe cancellation tokens or an explicit
cancellation API should be added; reference ch, callbackID and deleteCallback in
the doc so callers know why the Rust task continues after ctx cancellation.
- Around line 13-15: The BamlRuntime.ptr field is unused—remove the sentinel
field and any assignments to it in NewRuntime, and update related code
(BamlRuntime struct definition and NewRuntime) so callers and CallFunction use
the remaining fields/methods without relying on ptr; alternatively, if you want
to preserve a sentinel, add a clear comment above the ptr field explaining its
purpose and ensure NewRuntime/CallFunction reference it (or a no-op cast) so
it's intentionally documented rather than left unused.
In `@baml_language/crates/bridge_go/pkg/tests/call_function_test.go`:
- Around line 64-77: The package-level variable testRT is lazily initialized in
getTestRuntime and can race if tests run in parallel; replace the ad-hoc nil
check with a sync.Once-based initialization (declare a once variable and use
once.Do to call the pkg.NewRuntime creation path) so getTestRuntime uses once.Do
to create testRT (and capture/handle any error inside the once body or propagate
it safely), referencing testRT, getTestRuntime, and pkg.NewRuntime to locate
where to apply the change.
- Around line 135-142: Replace the unchecked type assertions that can
panic—change result.(map[string]any) to a comma-ok checked assertion (e.g., m,
ok := result.(map[string]any); if !ok { t.Fatalf("expected result to be
map[string]any, got %T", result) }) and likewise change addr :=
m["address"].(map[string]any) to a checked form (addr, ok :=
m["address"].(map[string]any); if !ok { t.Fatalf("expected m[\"address\"] to be
map[string]any, got %T", m["address"]) }), and also validate the types of
m["name"] and addr["zip"] before comparing values to avoid panics.
In `@baml_language/crates/bridge_go/pkg/tests/handle_test.go`:
- Around line 9-23: Add a positive-path test that exercises the successful
handle lifecycle: in the same TestCloneAndReleaseHandle (or a new
TestCloneAndReleaseHandle_Success) obtain a runtime-backed valid pkg.BamlHandle
(e.g., call the package factory or function that returns a live handle instead
of constructing Key:0), assert the returned handle.Key is non-zero, call Clone()
on that handle and assert cloned.Key equals the original and is non-zero, then
call Release() on both original and clone and ensure no panics or errors;
reference the pkg.BamlHandle type and its Clone and Release methods when adding
the assertions.
In `@baml_language/crates/bridge_go/src/callbacks.rs`:
- Around line 11-18: The register_callbacks function currently ignores
OnceCell::set() errors which silently hides double-registration; update
register_callbacks to check the Result returned by RESULT_CALLBACK_FN.set(...)
and ERROR_CALLBACK_FN.set(...), and when set returns Err (meaning already
initialized) either emit a debug-only assertion (e.g., debug_assert!(false,
"register_callbacks called twice for RESULT_CALLBACK_FN")) or write a debug log
(e.g., eprintln! or crate logger) that includes the symbol name and the
existing/attempted state so double-initialization is visible during development;
keep the public API unchanged but ensure the check references
register_callbacks, RESULT_CALLBACK_FN, ERROR_CALLBACK_FN and OnceCell::set to
locate the code.
In `@baml_language/crates/bridge_go/src/errors.rs`:
- Around line 3-60: Add unit tests in errors.rs to assert stable string prefixes
and contents produced by bridge_error_to_string and runtime_error_to_string:
create tests for BridgeError::NotInitialized, BridgeError::FunctionNotFound {
name }, BridgeError::MissingArgument { function, parameter },
BridgeError::Internal(msg), BridgeError::Ctypes, and a RuntimeError case (e.g.,
RuntimeError::InvalidArgument and EngineError::Cancelled /
EngineError::FunctionNotFound) to ensure strings start with the expected
category tokens like "BamlError: BamlInvalidArgumentError:" or "BamlError:
BamlClientError:" and contain the specific details; call bridge_error_to_string
and runtime_error_to_string directly and use assertions like starts_with and
contains to lock the format.
In `@baml_language/crates/bridge_go/src/functions.rs`:
- Around line 1-98: Add Rust unit tests for this module by creating a
#[cfg(test)] mod tests that exercises the pure/testable parts: 1) test
call_function_inner returns Err(BridgeError::NullFunctionName) when
function_name is null, 2) test decoding of CallFunctionArgs from a known buffer
and conversion via kwargs_to_bex_values to ensure argument decoding succeeds,
and 3) test encode_success() and encode_error() produce CallAck protobufs with
expected response variants (decode the Buffer back to CallAck and assert the
response). Use the existing symbols call_function_inner, CallFunctionArgs,
kwargs_to_bex_values, encode_success, and encode_error and mock or construct
minimal inputs (e.g., empty/default CallFunctionArgs) so tests do not require
the async runtime or external services. Ensure tests live in the same file under
a tests module and assert expected Result or decoded protobuf values.
In `@baml_language/crates/bridge_go/src/handle.rs`:
- Around line 3-11: Add Rust unit tests in this file under a #[cfg(test)] mod to
lock the FFI contracts: write one test that calls clone_handle with a
known-missing key and asserts it returns 0 (use HANDLE_TABLE keys that are not
inserted or a very large u64), and write another test that calls release_handle
with an invalid/nonexistent key and verifies it does not panic (use
std::panic::catch_unwind and assert the call completes). Reference the exported
functions clone_handle and release_handle and the HANDLE_TABLE symbol in the
tests so the ABI behaviors are codified.
In `@baml_language/crates/bridge_go/src/runtime.rs`:
- Around line 30-32: The use of std::ptr::dangling::<libc::c_void>() as a
sentinel pointer in runtime.rs is unclear to future readers; replace the inline
call with a clearly named sentinel (e.g., RUNTIME_SENTINEL) or add a short doc
comment on the function that returns it explaining that it is a well-aligned,
non-null sentinel used because the runtime is global; update the return site
where std::ptr::dangling::<libc::c_void>() is currently used to reference the
named constant or add the doc comment above the function so maintainers see the
intent immediately.
🪄 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: a3403ebb-8861-497e-a2d6-1a5af71af581
⛔ Files ignored due to path filters (4)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/bridge_go/cffi/proto/baml/cffi/v1/baml_inbound.pb.gois excluded by!**/*.pb.gobaml_language/crates/bridge_go/cffi/proto/baml/cffi/v1/baml_outbound.pb.gois excluded by!**/*.pb.gobaml_language/crates/bridge_go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
baml_language/crates/bridge_go/Cargo.tomlbaml_language/crates/bridge_go/build.shbaml_language/crates/bridge_go/cffi/exports.gobaml_language/crates/bridge_go/cffi/lib.gobaml_language/crates/bridge_go/cffi/types.gobaml_language/crates/bridge_go/go.modbaml_language/crates/bridge_go/pkg/callbacks.gobaml_language/crates/bridge_go/pkg/errors.gobaml_language/crates/bridge_go/pkg/handle.gobaml_language/crates/bridge_go/pkg/init_callbacks.gobaml_language/crates/bridge_go/pkg/proto.gobaml_language/crates/bridge_go/pkg/runtime.gobaml_language/crates/bridge_go/pkg/tests/call_function_test.gobaml_language/crates/bridge_go/pkg/tests/engine_test.gobaml_language/crates/bridge_go/pkg/tests/handle_test.gobaml_language/crates/bridge_go/src/callbacks.rsbaml_language/crates/bridge_go/src/errors.rsbaml_language/crates/bridge_go/src/functions.rsbaml_language/crates/bridge_go/src/handle.rsbaml_language/crates/bridge_go/src/lib.rsbaml_language/crates/bridge_go/src/panic.rsbaml_language/crates/bridge_go/src/runtime.rs
Comment on lines
+11
to
+15
| if [ -n "${PROTOC_GEN_GO_PATH:-}" ]; then | ||
| export PATH="$(dirname "${PROTOC_GEN_GO_PATH}"):${PATH}" | ||
| elif command -v mise &>/dev/null && mise which protoc-gen-go &>/dev/null; then | ||
| export PATH="$(dirname "$(mise which protoc-gen-go)")":${PATH} | ||
| fi |
Contributor
There was a problem hiding this comment.
Declare and assign separately to avoid masking return values.
Shellcheck SC2155: When export and command substitution are combined, a non-zero exit from the subshell is masked by export's success.
🔧 Proposed fix
# Resolve protoc-gen-go
if [ -n "${PROTOC_GEN_GO_PATH:-}" ]; then
- export PATH="$(dirname "${PROTOC_GEN_GO_PATH}"):${PATH}"
+ protoc_gen_go_dir="$(dirname "${PROTOC_GEN_GO_PATH}")"
+ export PATH="${protoc_gen_go_dir}:${PATH}"
elif command -v mise &>/dev/null && mise which protoc-gen-go &>/dev/null; then
- export PATH="$(dirname "$(mise which protoc-gen-go)")":${PATH}
+ mise_protoc_path="$(mise which protoc-gen-go)"
+ export PATH="$(dirname "${mise_protoc_path}"):${PATH}"
fi🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 12-12: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 14-14: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@baml_language/crates/bridge_go/build.sh` around lines 11 - 15, The paired
export+command-substitution in build.sh masks command failures (SC2155); split
the assignment and export so failures surface: assign the dirname result to a
local variable (e.g., protoc_dir or mise_dir) using command substitution (for
both the PROTOC_GEN_GO_PATH branch and the mise branch that calls `mise which
protoc-gen-go`), check the command succeeded if needed, then export
PATH="$protoc_dir:$PATH" (or export PATH="$mise_dir:$PATH"); update the two
blocks around the export PATH lines to use these separate assignment-then-export
steps so non-zero exits are not masked.
Comment on lines
+107
to
+109
| if result != "red" { | ||
| t.Fatalf("expected 'Red', got %v", result) | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent error message vs assertion value.
The assertion checks for "red" (lowercase) but the error message says "expected 'Red'" (capitalized). This will confuse developers when the test fails.
🐛 Fix the error message
- if result != "red" {
- t.Fatalf("expected 'Red', got %v", result)
+ if result != "red" {
+ t.Fatalf("expected 'red', got %v", result)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if result != "red" { | |
| t.Fatalf("expected 'Red', got %v", result) | |
| } | |
| if result != "red" { | |
| t.Fatalf("expected 'red', got %v", result) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@baml_language/crates/bridge_go/pkg/tests/call_function_test.go` around lines
107 - 109, The failure message is inconsistent with the asserted value: the test
asserts that result == "red" but t.Fatalf says "expected 'Red'"; update the
t.Fatalf call that references result to match the asserted lowercase value
(e.g., "expected 'red', got %v") so the error message accurately reflects the
expectation; locate the t.Fatalf call in call_function_test.go that checks the
variable result and adjust its message accordingly.
Comment on lines
+16
to
+22
| ffi_safe_ptr(|| { | ||
| let root_path_str = unsafe { CStr::from_ptr(root_path) } | ||
| .to_str() | ||
| .map_err(|e| format!("Invalid root_path UTF-8: {e}"))?; | ||
| let src_files_str = unsafe { CStr::from_ptr(src_files_json) } | ||
| .to_str() | ||
| .map_err(|e| format!("Invalid src_files_json UTF-8: {e}"))?; |
Contributor
There was a problem hiding this comment.
Add null pointer checks before CStr::from_ptr.
If the Go side passes a null pointer (e.g., due to a bug or empty string handling), CStr::from_ptr will dereference null, causing undefined behavior. While ffi_safe_ptr catches panics, null dereference may not panic on all platforms—it can cause a segfault.
🛡️ Proposed fix
ffi_safe_ptr(|| {
+ if root_path.is_null() {
+ return Err("root_path is null".to_string());
+ }
+ if src_files_json.is_null() {
+ return Err("src_files_json is null".to_string());
+ }
let root_path_str = unsafe { CStr::from_ptr(root_path) }
.to_str()
.map_err(|e| format!("Invalid root_path UTF-8: {e}"))?;
let src_files_str = unsafe { CStr::from_ptr(src_files_json) }
.to_str()
.map_err(|e| format!("Invalid src_files_json UTF-8: {e}"))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ffi_safe_ptr(|| { | |
| let root_path_str = unsafe { CStr::from_ptr(root_path) } | |
| .to_str() | |
| .map_err(|e| format!("Invalid root_path UTF-8: {e}"))?; | |
| let src_files_str = unsafe { CStr::from_ptr(src_files_json) } | |
| .to_str() | |
| .map_err(|e| format!("Invalid src_files_json UTF-8: {e}"))?; | |
| ffi_safe_ptr(|| { | |
| if root_path.is_null() { | |
| return Err("root_path is null".to_string()); | |
| } | |
| if src_files_json.is_null() { | |
| return Err("src_files_json is null".to_string()); | |
| } | |
| let root_path_str = unsafe { CStr::from_ptr(root_path) } | |
| .to_str() | |
| .map_err(|e| format!("Invalid root_path UTF-8: {e}"))?; | |
| let src_files_str = unsafe { CStr::from_ptr(src_files_json) } | |
| .to_str() | |
| .map_err(|e| format!("Invalid src_files_json UTF-8: {e}"))?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@baml_language/crates/bridge_go/src/runtime.rs` around lines 16 - 22, Add
explicit null-pointer checks for the incoming C pointers before calling
CStr::from_ptr: inside the ffi_safe_ptr closure, test if root_path.is_null() and
src_files_json.is_null() and return an Err with a descriptive message (e.g.,
format!("Null pointer for root_path") / format!("Null pointer for
src_files_json")) instead of calling unsafe CStr::from_ptr; then proceed to call
unsafe CStr::from_ptr(...) only after each pointer is confirmed non-null. Ensure
you reference the existing symbols ffi_safe_ptr, root_path, src_files_json and
CStr::from_ptr when making the changes so the early-return error integrates with
the surrounding Result flow.
Merging this PR will not alter performance
|
Binary size checks passed✅ 7 passed
Generated by |
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.
Summary by CodeRabbit
Release Notes