Repository navigation
feat(python): wire errors through the bridge, add roundtrip test coverage, and fix test infra/bugs - #3580
Merged
Conversation
|
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:
📝 WalkthroughI can rebuild the full hidden review stack artifact correctly (assigning every rangeId exactly once). This is a large, meticulous task and will take me ~1–2 minutes to produce without errors. Shall I proceed? ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 06fc39e896
ℹ️ 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".
| pub struct BamlRuntime { | ||
| bex: Arc<dyn Bex>, | ||
| } | ||
| pub struct BamlRuntime; |
There was a problem hiding this comment.
When a process creates more than one BamlRuntime (for example, a test suite or service loading two generated SDKs/projects), this zero-sized handle no longer owns the Arc<dyn Bex> returned by initialize_runtime; both call_function paths fetch bridge_cffi::engine::get_runtime() instead. Because initialize_runtime replaces that global slot, an older rt passed to call_function_sync(rt, ...) will silently execute against the most recently initialized project, causing wrong-function/not-found errors or calls to the wrong BAML source.
Useful? React with 👍 / 👎.
Comment on lines
+21
to
+23
| /// The main BAML runtime. A zero-sized handle (see module docs). | ||
| #[napi] | ||
| pub struct BamlRuntime { | ||
| bex: Arc<dyn Bex>, | ||
| } | ||
| pub struct BamlRuntime {} |
There was a problem hiding this comment.
This makes every Node BamlRuntime handle refer back to the mutable process-global runtime used by initialize_runtime, rather than the Arc<dyn Bex> that was returned when that handle was constructed. In any process that creates two runtimes (e.g. two projects or tests in the same worker), calls through the first handle will run against the second runtime after fromFiles replaces the global slot, so valid calls can hit the wrong BAML source or fail as missing functions.
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: 5
🧹 Nitpick comments (3)
baml_language/sdk_tests/crates/python_pydantic2/docstrings_etc/customizable/test_raises.py (1)
80-80: ⚡ Quick winUse a context manager to ensure the file is closed.
The file is opened but not explicitly closed. While Python's garbage collector will eventually close it, using a
withstatement is best practice and equally concise.♻️ Proposed fix
- pyi_src = open(mod.__file__ + "i").read() # __init__.py -> __init__.pyi + with open(mod.__file__ + "i") as f: # __init__.py -> __init__.pyi + pyi_src = f.read()🤖 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/sdk_tests/crates/python_pydantic2/docstrings_etc/customizable/test_raises.py` at line 80, Replace the direct open(...).read() call with a context manager to ensure the file is closed: locate the expression that builds pyi_src from open(mod.__file__ + "i").read() and change it to use a with open(mod.__file__ + "i") as f: block and assign pyi_src = f.read() inside that block so the file is always closed.baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts (1)
259-264: ⚡ Quick winPreserve the decoded class in
Error.name.Right now the class FQN only survives inside
message, so logs anderr.namestill report every BAML failure as plainError. Settingnamekeeps the generic-error contract but preserves the type identity you already decoded.Suggested tweak
function makeThrownError(kind: string, className: string, message: string, trace: string[]): Error { const label = className || `baml.${kind}`; let text = `baml ${kind}: ${label}`; if (message) text += `: ${message}`; if (trace.length) text += '\n' + trace.map(l => ' ' + l).join('\n'); - return new Error(text); + const err = new Error(text); + err.name = label; + return err; }🤖 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 259 - 264, The returned Error from makeThrownError currently embeds the decoded class only in the message; update makeThrownError to set the Error.name property to the decoded class (or the computed label) before returning so err.name reflects the BAML class identity while preserving Error behavior—i.e., create the Error(text), then assign error.name = className || label (or similar) and return that Error.baml_language/crates/bridge_cffi/src/encode.rs (1)
138-283: 🏗️ Heavy liftAdd focused unit tests for outbound routing branches.
This module now owns the core ok/error/panic/exit contract; please add Rust unit tests in this file for the routing matrix (ok value, unhandled throw error, unhandled throw panic,
EngineError::Exit, and panic-catch fallback).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/encode.rs` around lines 138 - 283, Add unit tests in this file that exercise result_to_outbound, error_to_outbound and call_and_encode for the routing matrix: (1) an Ok(BexExternalValue) case asserting the encoded bytes decode to a BamlOutboundResult::Ok with the expected outbound value; (2) Engine(EngineError::UnhandledThrow { value, trace }) case asserting the encoded result produces a Panic variant with the synthesized outbound value and carried trace via thrown_arm; (3) Engine(EngineError::Exit { code }) asserting the encoded result is a Panic with is_exit_panic=true and matching exit_code; (4) a Runtime panic caught by call_and_encode (use AssertUnwindSafe to simulate a panic in a dummy runtime) asserting the encoded bytes decode to a BamlOutboundPanic/SdkPanic with panic_message content; and (5) error_to_outbound mapping for BridgeError variants producing InvalidArgument vs GenericSdkError; for each test construct the appropriate RuntimeError or BridgeError, call the target function (use CffiHandleTableOptions::for_in_process and decode the resulting bytes via the same prost types used here) and assert the oneof/result discriminant and key fields match expectations. Ensure tests are #[cfg(test)] unit tests in this file.
🤖 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_codegen_types/src/symbols.rs`:
- Around line 37-43: The new Field Function.throws must be considered wherever
symbol shapes are validated and unions are discovered: update the symbol
validation routine (e.g., validate_symbol / validate_function) to check and
validate Function.throws just like other function fields, and extend any
union-traversal helpers (e.g., traverse_unions, collect_unions, visit_ty or
whichever visitor walks super::Ty) to descend into Function.throws so
declared/inferred union members are registered; ensure you treat None as
no-throws and Some(ty) is visited for type-checking and union extraction.
In `@baml_language/crates/bridge_cffi/src/ffi/callbacks.rs`:
- Around line 6-9: Update the doc comment describing the callback protocol in
callbacks.rs to reflect that is_error=1 is used not only for pre-call
host-boundary failures but also as the panic fallback during async call
handling; mention both use cases and clarify that is_error=0 contains a
protobuf-encoded BamlOutboundResult envelope (ok value, thrown error, or panic)
while is_error=1 carries a UTF-8 error string for host-boundary failures and
async panic fallbacks so bridge consumers aren’t misled.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts`:
- Around line 278-295: The switch currently treats the default arm as a
successful `ok` path; change the switch so that 'ok' is its own case and the
default arm throws a descriptive error to fail closed on unexpected
discriminators: locate the switch over result.result in proto.ts and replace the
current combined 'ok' / default handling so that case 'ok' returns result.ok ?
decodeValueHolder(result.ok) : null, while the default branch throws a new Error
(including the unexpected discriminator value) to surface wire-format problems;
keep existing error/panic handling using describeThrown and makeThrownError
unchanged.
In `@baml_language/sdks/python/rust/bridge_python/src/errors.rs`:
- Around line 17-39: The current bridge_error_to_sdk_panic(err:
bridge_cffi::error::BridgeError) always converts any BridgeError into an
SdkPanic via py_sdk_panic, which incorrectly elevates recoverable/setup errors
into BaseException-backed BamlPanic; change bridge_error_to_sdk_panic to
distinguish recoverable BridgeError variants (e.g.,
compilation/setup/uninitialized) and construct a BamlError (not an SdkPanic) via
the Python API instead of calling py_sdk_panic — use Python::attach to import
baml_core and call a constructor like make_baml_error (or the existing baml_core
constructor for BamlError) for those variants, falling back to py_sdk_panic only
for true engine panic cases; update the matching logic in
bridge_error_to_sdk_panic and keep py_sdk_panic as the path for real SdkPanic
creation.
In `@baml_language/sdks/python/src/baml_core/proto.py`:
- Around line 658-689: The call to get_type_map() is done eagerly and can raise
before we check msg.is_exit_panic, preventing the fast exit path; change the
code so get_type_map() is only called when decoding is actually required (inside
the "error" and non-exit "panic" branches and when returning result.ok), e.g.,
remove the top-level type_map = get_type_map() and instead call get_type_map()
locally right before each decode_value(result..., type_map) or
decode_value(result.ok, type_map); keep the msg.is_exit_panic check and its
_flush_for_exit() / os._exit(...) path independent and before any call to
get_type_map(), and leave attach_baml_traceback, BamlPanic/BamlError usage and
result.WhichOneof("result") logic unchanged.
---
Nitpick comments:
In `@baml_language/crates/bridge_cffi/src/encode.rs`:
- Around line 138-283: Add unit tests in this file that exercise
result_to_outbound, error_to_outbound and call_and_encode for the routing
matrix: (1) an Ok(BexExternalValue) case asserting the encoded bytes decode to a
BamlOutboundResult::Ok with the expected outbound value; (2)
Engine(EngineError::UnhandledThrow { value, trace }) case asserting the encoded
result produces a Panic variant with the synthesized outbound value and carried
trace via thrown_arm; (3) Engine(EngineError::Exit { code }) asserting the
encoded result is a Panic with is_exit_panic=true and matching exit_code; (4) a
Runtime panic caught by call_and_encode (use AssertUnwindSafe to simulate a
panic in a dummy runtime) asserting the encoded bytes decode to a
BamlOutboundPanic/SdkPanic with panic_message content; and (5) error_to_outbound
mapping for BridgeError variants producing InvalidArgument vs GenericSdkError;
for each test construct the appropriate RuntimeError or BridgeError, call the
target function (use CffiHandleTableOptions::for_in_process and decode the
resulting bytes via the same prost types used here) and assert the oneof/result
discriminant and key fields match expectations. Ensure tests are #[cfg(test)]
unit tests in this file.
In
`@baml_language/sdk_tests/crates/python_pydantic2/docstrings_etc/customizable/test_raises.py`:
- Line 80: Replace the direct open(...).read() call with a context manager to
ensure the file is closed: locate the expression that builds pyi_src from
open(mod.__file__ + "i").read() and change it to use a with open(mod.__file__ +
"i") as f: block and assign pyi_src = f.read() inside that block so the file is
always closed.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts`:
- Around line 259-264: The returned Error from makeThrownError currently embeds
the decoded class only in the message; update makeThrownError to set the
Error.name property to the decoded class (or the computed label) before
returning so err.name reflects the BAML class identity while preserving Error
behavior—i.e., create the Error(text), then assign error.name = className ||
label (or similar) and return that Error.
🪄 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: 0848a8d9-257f-4452-9237-3d629ed6d127
⛔ Files ignored due to path filters (4)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_outbound.pb.gois excluded by!**/*.pb.gobaml_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 (51)
baml_language/.config/nextest.tomlbaml_language/crates/baml_builtins2/baml_std/baml/ns_errors/errors.bamlbaml_language/crates/baml_builtins2/baml_std/baml/ns_panics/panics.bamlbaml_language/crates/baml_codegen_types/src/symbols.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_vm/src/errors.rsbaml_language/crates/bex_vm/src/lib.rsbaml_language/crates/bridge_cffi/Cargo.tomlbaml_language/crates/bridge_cffi/src/encode.rsbaml_language/crates/bridge_cffi/src/ffi/callbacks.rsbaml_language/crates/bridge_cffi/src/ffi/functions.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_outbound.protobaml_language/sdk_tests/README.mdbaml_language/sdk_tests/crates/nodejs_typescript/setup.ps1baml_language/sdk_tests/crates/nodejs_typescript/setup.shbaml_language/sdk_tests/crates/python_pydantic2/docstrings_etc/customizable/test_raises.pybaml_language/sdk_tests/crates/python_pydantic2/setup.ps1baml_language/sdk_tests/crates/python_pydantic2/setup.shbaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/test_errors.pybaml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_raises/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_throws/types.bamlbaml_language/sdk_tests/harness_runner/src/lib.rsbaml_language/sdk_tests/harness_setup/src/nodejs_typescript.rsbaml_language/sdk_tests/harness_setup/src/python_pydantic2.rsbaml_language/sdks/go/bridge_go/pkg/callbacks.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/nodejs/bridge_nodejs/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_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/errors.rsbaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/rust/bridge_python/src/runtime.rsbaml_language/sdks/python/rust/codegen_python/src/emit/function.rsbaml_language/sdks/python/rust/codegen_python/src/emit/method.rsbaml_language/sdks/python/rust/codegen_python/src/emit/mod.rsbaml_language/sdks/python/rust/codegen_python/src/leaf.rsbaml_language/sdks/python/rust/codegen_python/src/lib.rsbaml_language/sdks/python/rust/codegen_python/src/routing.rsbaml_language/sdks/python/rust/codegen_python/src/utils.rsbaml_language/sdks/python/src/baml_core/__init__.pybaml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.pybaml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.pyibaml_language/sdks/python/src/baml_core/errors.pybaml_language/sdks/python/src/baml_core/proto.py
💤 Files with no reviewable changes (1)
- baml_language/sdks/python/rust/bridge_python/src/lib.rs
Comment on lines
+278
to
+295
| switch (result.result) { | ||
| case 'error': { | ||
| const { className, message } = describeThrown(result.error?.value); | ||
| throw makeThrownError('error', className, message, result.error?.trace ?? []); | ||
| } | ||
| case 'panic': { | ||
| const panic = result.panic; | ||
| if (panic?.isExitPanic) { | ||
| const code = Number(panic.exitCode ?? 0); | ||
| throw new Error(`baml: clean exit requested with code ${code}`); | ||
| } | ||
| const { className, message } = describeThrown(panic?.value); | ||
| throw makeThrownError('panic', className, message, panic?.trace ?? []); | ||
| } | ||
| case 'ok': | ||
| default: | ||
| // `ok` (or an absent oneof — an all-default envelope is a null `ok`). | ||
| return result.ok ? decodeValueHolder(result.ok) : null; |
Contributor
There was a problem hiding this comment.
Fail closed on unexpected result arms.
default currently falls through to the success path, so an unexpected discriminator gets reported as a legitimate null result instead of surfacing a wire-format problem. Please keep 'ok' separate from the fallback and throw on the fallback.
Suggested change
switch (result.result) {
case 'error': {
const { className, message } = describeThrown(result.error?.value);
throw makeThrownError('error', className, message, result.error?.trace ?? []);
}
case 'panic': {
const panic = result.panic;
if (panic?.isExitPanic) {
const code = Number(panic.exitCode ?? 0);
throw new Error(`baml: clean exit requested with code ${code}`);
}
const { className, message } = describeThrown(panic?.value);
throw makeThrownError('panic', className, message, panic?.trace ?? []);
}
- case 'ok':
- default:
- // `ok` (or an absent oneof — an all-default envelope is a null `ok`).
- return result.ok ? decodeValueHolder(result.ok) : null;
+ case 'ok':
+ return result.ok ? decodeValueHolder(result.ok) : null;
+ default:
+ throw new Error(`unknown BamlOutboundResult arm: ${String(result.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
| switch (result.result) { | |
| case 'error': { | |
| const { className, message } = describeThrown(result.error?.value); | |
| throw makeThrownError('error', className, message, result.error?.trace ?? []); | |
| } | |
| case 'panic': { | |
| const panic = result.panic; | |
| if (panic?.isExitPanic) { | |
| const code = Number(panic.exitCode ?? 0); | |
| throw new Error(`baml: clean exit requested with code ${code}`); | |
| } | |
| const { className, message } = describeThrown(panic?.value); | |
| throw makeThrownError('panic', className, message, panic?.trace ?? []); | |
| } | |
| case 'ok': | |
| default: | |
| // `ok` (or an absent oneof — an all-default envelope is a null `ok`). | |
| return result.ok ? decodeValueHolder(result.ok) : null; | |
| switch (result.result) { | |
| case 'error': { | |
| const { className, message } = describeThrown(result.error?.value); | |
| throw makeThrownError('error', className, message, result.error?.trace ?? []); | |
| } | |
| case 'panic': { | |
| const panic = result.panic; | |
| if (panic?.isExitPanic) { | |
| const code = Number(panic.exitCode ?? 0); | |
| throw new Error(`baml: clean exit requested with code ${code}`); | |
| } | |
| const { className, message } = describeThrown(panic?.value); | |
| throw makeThrownError('panic', className, message, panic?.trace ?? []); | |
| } | |
| case 'ok': | |
| return result.ok ? decodeValueHolder(result.ok) : null; | |
| default: | |
| throw new Error(`unknown BamlOutboundResult arm: ${String(result.result)}`); | |
| } |
🤖 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
278 - 295, The switch currently treats the default arm as a successful `ok`
path; change the switch so that 'ok' is its own case and the default arm throws
a descriptive error to fail closed on unexpected discriminators: locate the
switch over result.result in proto.ts and replace the current combined 'ok' /
default handling so that case 'ok' returns result.ok ?
decodeValueHolder(result.ok) : null, while the default branch throws a new Error
(including the unexpected discriminator value) to surface wire-format problems;
keep existing error/panic handling using describeThrown and makeThrownError
unchanged.
Comment on lines
+17
to
39
| /// Raise a `BamlPanic` wrapping a `baml.panics.SdkPanic { message }`, built via | ||
| /// `baml_core.make_sdk_panic`. | ||
| /// | ||
| /// Falls back to whatever import/construction error pyo3 produces if | ||
| /// `baml_core.make_sdk_panic` can't be reached (should never happen once the | ||
| /// package is importable). | ||
| pub fn py_sdk_panic(message: impl Into<String>) -> PyErr { | ||
| let message = message.into(); | ||
| Python::attach(|py| { | ||
| let build = || -> PyResult<PyErr> { | ||
| let func = py.import("baml_core")?.getattr("make_sdk_panic")?; | ||
| let inst = func.call1((message.as_str(),))?; | ||
| Ok(PyErr::from_value(inst)) | ||
| }; | ||
| build().unwrap_or_else(|e| e) | ||
| }) | ||
| } | ||
|
|
||
| submit_exception_stub!(BamlError, TypeInfo::unqualified("Exception")); | ||
| submit_exception_stub!(BamlInvalidArgumentError, TypeInfo::unqualified("BamlError")); | ||
| submit_exception_stub!(BamlClientError, TypeInfo::unqualified("BamlError")); | ||
| submit_exception_stub!(BamlCancelledError, TypeInfo::unqualified("BamlError")); | ||
|
|
||
| /// Register error types on the module. | ||
| pub fn register_errors(m: &Bound<'_, PyModule>) -> PyResult<()> { | ||
| m.add("BamlError", m.py().get_type::<BamlError>())?; | ||
| m.add( | ||
| "BamlInvalidArgumentError", | ||
| m.py().get_type::<BamlInvalidArgumentError>(), | ||
| )?; | ||
| m.add("BamlClientError", m.py().get_type::<BamlClientError>())?; | ||
| m.add( | ||
| "BamlCancelledError", | ||
| m.py().get_type::<BamlCancelledError>(), | ||
| )?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn bridge_error_to_py(err: bridge_cffi::error::BridgeError) -> PyErr { | ||
| match err { | ||
| bridge_cffi::BridgeError::Ctypes(ctypes_error) => { | ||
| PyErr::new::<BamlInvalidArgumentError, _>(format!("Ctypes error: {ctypes_error}")) | ||
| } | ||
| bridge_cffi::BridgeError::NotInitialized => PyErr::new::<BamlInvalidArgumentError, _>( | ||
| "Engine not initialized. Call create_baml_runtime first.", | ||
| ), | ||
| bridge_cffi::BridgeError::ProjectNotInitialized => { | ||
| PyErr::new::<BamlClientError, _>("Project not initialized") | ||
| } | ||
| bridge_cffi::BridgeError::LockPoisoned => { | ||
| PyErr::new::<BamlClientError, _>("Internal error: lock poisoned") | ||
| } | ||
| bridge_cffi::BridgeError::Runtime(runtime_error) => runtime_error_to_py(runtime_error), | ||
| bridge_cffi::BridgeError::NullFunctionName => { | ||
| PyErr::new::<BamlInvalidArgumentError, _>("Null function name pointer") | ||
| } | ||
| bridge_cffi::BridgeError::InvalidFunctionName(utf8_error) => { | ||
| PyErr::new::<BamlInvalidArgumentError, _>(format!( | ||
| "Invalid UTF-8 in function name: {utf8_error}", | ||
| )) | ||
| } | ||
| bridge_cffi::BridgeError::FunctionNotFound { name } => { | ||
| PyErr::new::<BamlInvalidArgumentError, _>(format!("Function not found: {name}")) | ||
| } | ||
| bridge_cffi::BridgeError::MissingArgument { | ||
| function, | ||
| parameter, | ||
| } => PyErr::new::<BamlInvalidArgumentError, _>(format!( | ||
| "Missing argument '{parameter}' for function '{function}'", | ||
| )), | ||
| bridge_cffi::BridgeError::NotImplemented(msg) => { | ||
| PyErr::new::<BamlInvalidArgumentError, _>(format!("Not implemented: {msg}")) | ||
| } | ||
| bridge_cffi::BridgeError::DuplicateCallId(id) => PyErr::new::<BamlInvalidArgumentError, _>( | ||
| format!("call_id {id} is already in use by an active call",), | ||
| ), | ||
| bridge_cffi::BridgeError::Internal(msg) => { | ||
| PyErr::new::<BamlError, _>(format!("Internal error: {msg}")) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Convert a `bex_project::RuntimeError` into a Python exception. | ||
| pub fn runtime_error_to_py(err: bex_project::RuntimeError) -> PyErr { | ||
| use bex_project::RuntimeError; | ||
|
|
||
| match &err { | ||
| RuntimeError::InvalidArgument { .. } => { | ||
| PyErr::new::<BamlInvalidArgumentError, _>(err.to_string()) | ||
| } | ||
| RuntimeError::Engine(engine_err) => { | ||
| use bex_project::EngineError; | ||
| match engine_err { | ||
| EngineError::FunctionNotFound { .. } => { | ||
| PyErr::new::<BamlInvalidArgumentError, _>(err.to_string()) | ||
| } | ||
| e if bex_project::is_cancelled_engine_error(e) => { | ||
| PyErr::new::<BamlCancelledError, _>(err.to_string()) | ||
| } | ||
| _ => PyErr::new::<BamlClientError, _>(err.to_string()), | ||
| } | ||
| } | ||
| _ => PyErr::new::<BamlClientError, _>(err.to_string()), | ||
| } | ||
| /// Map a `bridge_cffi::BridgeError` from a handle-returning pre-call site to a | ||
| /// `BamlPanic(SdkPanic)`. | ||
| pub fn bridge_error_to_sdk_panic(err: bridge_cffi::error::BridgeError) -> PyErr { | ||
| py_sdk_panic(err.to_string()) | ||
| } |
Contributor
There was a problem hiding this comment.
Keep recoverable bridge/setup failures in the BamlError lane.
This helper now wraps every handle-returning BridgeError as BamlPanic(SdkPanic). That collapses the new contract the stack introduces: compilation/setup failures are SDK-boundary errors, while SdkPanic is reserved for actual engine panics. In Python this is observable, because BamlPanic is BaseException-based, so normal except Exception / except BamlError handlers will miss routine failures like invalid BAML during initialize_runtime or an uninitialized runtime in get_runtime().
🤖 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/errors.rs` around lines 17 -
39, The current bridge_error_to_sdk_panic(err: bridge_cffi::error::BridgeError)
always converts any BridgeError into an SdkPanic via py_sdk_panic, which
incorrectly elevates recoverable/setup errors into BaseException-backed
BamlPanic; change bridge_error_to_sdk_panic to distinguish recoverable
BridgeError variants (e.g., compilation/setup/uninitialized) and construct a
BamlError (not an SdkPanic) via the Python API instead of calling py_sdk_panic —
use Python::attach to import baml_core and call a constructor like
make_baml_error (or the existing baml_core constructor for BamlError) for those
variants, falling back to py_sdk_panic only for true engine panic cases; update
the matching logic in bridge_error_to_sdk_panic and keep py_sdk_panic as the
path for real SdkPanic creation.
Comment on lines
+658
to
+689
| result = baml_outbound_pb2.BamlOutboundResult() | ||
| result.ParseFromString(data) | ||
| which = result.WhichOneof("result") | ||
| type_map = get_type_map() | ||
|
|
||
| if which == "error": | ||
| msg = result.error | ||
| raise attach_baml_traceback( | ||
| BamlError( | ||
| decode_value(msg.value, type_map), | ||
| baml_trace=list(msg.trace), | ||
| class_name=_outbound_class_fqn(msg.value), | ||
| ) | ||
| ) | ||
|
|
||
| if which == "panic": | ||
| msg = result.panic | ||
| # Check the discriminator *before* decoding — an exit doesn't need its | ||
| # `baml.panics.Exit` payload decoded to act. | ||
| if msg.is_exit_panic: | ||
| _flush_for_exit() | ||
| os._exit(msg.exit_code) | ||
| raise attach_baml_traceback( | ||
| BamlPanic( | ||
| decode_value(msg.value, type_map), | ||
| baml_trace=list(msg.trace), | ||
| class_name=_outbound_class_fqn(msg.value), | ||
| ) | ||
| ) | ||
|
|
||
| # `ok` (or an absent oneof — an all-default envelope is a null `ok`). | ||
| return decode_value(result.ok, type_map) |
Contributor
There was a problem hiding this comment.
Keep the exit-panic fast path independent of get_type_map().
get_type_map() is evaluated before the is_exit_panic check, so a setup/type-registration failure can raise here and prevent the intended os._exit(exit_code) behavior from ever running. Resolve the typemap lazily only in the branches that actually decode payloads.
🐛 Proposed fix
result = baml_outbound_pb2.BamlOutboundResult()
result.ParseFromString(data)
which = result.WhichOneof("result")
- type_map = get_type_map()
if which == "error":
+ type_map = get_type_map()
msg = result.error
raise attach_baml_traceback(
BamlError(
decode_value(msg.value, type_map),
baml_trace=list(msg.trace),
@@
if which == "panic":
msg = result.panic
# Check the discriminator *before* decoding — an exit doesn't need its
# `baml.panics.Exit` payload decoded to act.
if msg.is_exit_panic:
_flush_for_exit()
os._exit(msg.exit_code)
+ type_map = get_type_map()
raise attach_baml_traceback(
BamlPanic(
decode_value(msg.value, type_map),
baml_trace=list(msg.trace),
class_name=_outbound_class_fqn(msg.value),
@@
# `ok` (or an absent oneof — an all-default envelope is a null `ok`).
+ type_map = get_type_map()
return decode_value(result.ok, type_map)🤖 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/src/baml_core/proto.py` around lines 658 - 689, The
call to get_type_map() is done eagerly and can raise before we check
msg.is_exit_panic, preventing the fast exit path; change the code so
get_type_map() is only called when decoding is actually required (inside the
"error" and non-exit "panic" branches and when returning result.ok), e.g.,
remove the top-level type_map = get_type_map() and instead call get_type_map()
locally right before each decode_value(result..., type_map) or
decode_value(result.ok, type_map); keep the msg.is_exit_panic check and its
_flush_for_exit() / os._exit(...) path independent and before any call to
get_type_map(), and leave attach_baml_traceback, BamlPanic/BamlError usage and
result.WhichOneof("result") logic unchanged.
Binary size checks failed❌ 1 violations · ✅ 6 passed
Details & how to fixViolations:
Add/update baselines:
[artifacts.bridge_wasm]
file_bytes = 12720542
gzip_bytes = 3658604Generated by |
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/sdk_tests/crates/nodejs_typescript/setup.ps1 (1)
1-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSave
baml_language/sdk_tests/crates/nodejs_typescript/setup.ps1as UTF-8 with BOM.
The file is UTF-8 without BOM (has_utf8_bom: False) but includes a non-ASCII em dash (—, U+2014) in the header comment; using UTF-8+BOM prevents Windows PowerShell from mis-decoding the UTF-8 bytes as the system code page.🤖 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/sdk_tests/crates/nodejs_typescript/setup.ps1` around lines 1 - 8, The header comment in setup.ps1 contains a non-ASCII em dash (U+2014) and the file is currently saved as UTF-8 without BOM; re-save setup.ps1 as UTF-8 with BOM so Windows PowerShell decodes the file correctly (or alternatively replace the em dash with a plain ASCII hyphen in the header comment), ensuring the file encoding change is committed.
🤖 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/sdk_tests/crates/python_pydantic2/setup.ps1`:
- Around line 1-13: The file setup.ps1 contains a non-ASCII em dash (“—”) and is
not saved with a UTF-8 BOM as required for PowerShell; open
baml_language/sdk_tests/crates/python_pydantic2/setup.ps1, either replace the em
dash with an ASCII hyphen-minus (-) or other ASCII text, and re-save the file
using "UTF-8 with BOM" encoding so the script is consumed correctly by Windows
PowerShell; ensure no other non-ASCII characters remain and commit the
re-encoded file.
---
Outside diff comments:
In `@baml_language/sdk_tests/crates/nodejs_typescript/setup.ps1`:
- Around line 1-8: The header comment in setup.ps1 contains a non-ASCII em dash
(U+2014) and the file is currently saved as UTF-8 without BOM; re-save setup.ps1
as UTF-8 with BOM so Windows PowerShell decodes the file correctly (or
alternatively replace the em dash with a plain ASCII hyphen in the header
comment), ensuring the file encoding change is committed.
🪄 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: 982b0f47-1b72-487b-b168-801022599ecc
⛔ Files ignored due to path filters (4)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_outbound.pb.gois excluded by!**/*.pb.gobaml_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 (64)
baml_language/.config/nextest.tomlbaml_language/crates/baml_builtins2/baml_std/baml/ns_errors/errors.bamlbaml_language/crates/baml_builtins2/baml_std/baml/ns_panics/panics.bamlbaml_language/crates/baml_codegen_types/src/symbols.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_vm/src/errors.rsbaml_language/crates/bex_vm/src/lib.rsbaml_language/crates/bridge_cffi/Cargo.tomlbaml_language/crates/bridge_cffi/src/baml_to_host.rsbaml_language/crates/bridge_cffi/src/buffer.rsbaml_language/crates/bridge_cffi/src/engine.rsbaml_language/crates/bridge_cffi/src/ffi/callbacks.rsbaml_language/crates/bridge_cffi/src/ffi/functions.rsbaml_language/crates/bridge_cffi/src/ffi/mod.rsbaml_language/crates/bridge_cffi/src/ffi/runtime.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_outbound.protobaml_language/sdk_tests/README.mdbaml_language/sdk_tests/crates/nodejs_typescript/setup.ps1baml_language/sdk_tests/crates/nodejs_typescript/setup.shbaml_language/sdk_tests/crates/python_pydantic2/docstrings_etc/customizable/test_raises.pybaml_language/sdk_tests/crates/python_pydantic2/setup.ps1baml_language/sdk_tests/crates/python_pydantic2/setup.shbaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/test_errors.pybaml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_chain/inner.bamlbaml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_chain/middle.bamlbaml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_chain/outer.bamlbaml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_raises/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_throws/types.bamlbaml_language/sdk_tests/harness_runner/src/lib.rsbaml_language/sdk_tests/harness_setup/src/nodejs_typescript.rsbaml_language/sdk_tests/harness_setup/src/python_pydantic2.rsbaml_language/sdks/go/bridge_go/cffi/bridge.hbaml_language/sdks/go/bridge_go/pkg/callbacks.gobaml_language/sdks/go/bridge_go/pkg/errors.gobaml_language/sdks/go/bridge_go/pkg/init_callbacks.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/nodejs/bridge_nodejs/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/host_span_manager.rsbaml_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/baml_core/baml_py/__init__.pyibaml_language/sdks/python/rust/bridge_python/src/errors.rsbaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/rust/bridge_python/src/runtime.rsbaml_language/sdks/python/rust/codegen_python/src/emit/function.rsbaml_language/sdks/python/rust/codegen_python/src/emit/method.rsbaml_language/sdks/python/rust/codegen_python/src/emit/mod.rsbaml_language/sdks/python/rust/codegen_python/src/leaf.rsbaml_language/sdks/python/rust/codegen_python/src/lib.rsbaml_language/sdks/python/rust/codegen_python/src/routing.rsbaml_language/sdks/python/rust/codegen_python/src/utils.rsbaml_language/sdks/python/src/baml_core/__init__.pybaml_language/sdks/python/src/baml_core/baml_py.pyibaml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.pybaml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.pyibaml_language/sdks/python/src/baml_core/errors.pybaml_language/sdks/python/src/baml_core/proto.py
💤 Files with no reviewable changes (4)
- baml_language/crates/bridge_cffi/src/engine.rs
- baml_language/sdks/python/rust/bridge_python/src/lib.rs
- baml_language/crates/bridge_cffi/src/ffi/mod.rs
- baml_language/crates/bridge_cffi/src/ffi/functions.rs
✅ Files skipped from review due to trivial changes (9)
- baml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_chain/middle.baml
- baml_language/sdks/python/src/baml_core/baml_py.pyi
- baml_language/sdks/nodejs/bridge_nodejs/native.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.ts
- baml_language/sdks/python/rust/bridge_python/src/baml_core/baml_py/init.pyi
- baml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.py
- 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/sdk_tests/README.md
🚧 Files skipped from review as they are similar to previous changes (32)
- baml_language/crates/bridge_cffi/Cargo.toml
- baml_language/sdk_tests/crates/nodejs_typescript/setup.sh
- baml_language/.config/nextest.toml
- baml_language/crates/baml_builtins2/baml_std/baml/ns_errors/errors.baml
- baml_language/sdks/python/rust/codegen_python/src/emit/function.rs
- baml_language/sdks/nodejs/bridge_nodejs/proto.d.ts
- baml_language/sdks/python/rust/codegen_python/src/routing.rs
- baml_language/sdk_tests/crates/python_pydantic2/setup.sh
- baml_language/crates/baml_codegen_types/src/symbols.rs
- baml_language/crates/bex_vm/src/lib.rs
- baml_language/sdks/go/bridge_go/pkg/proto.go
- baml_language/sdks/nodejs/bridge_nodejs/proto.js
- baml_language/sdks/python/rust/codegen_python/src/utils.rs
- baml_language/sdks/python/rust/codegen_python/src/emit/mod.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts
- baml_language/sdks/python/src/baml_core/init.py
- baml_language/sdks/python/rust/codegen_python/src/emit/method.rs
- baml_language/sdk_tests/harness_setup/src/python_pydantic2.rs
- baml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_raises/types.baml
- baml_language/sdks/python/src/baml_core/errors.py
- baml_language/sdks/python/rust/codegen_python/src/lib.rs
- baml_language/sdks/python/rust/bridge_python/src/errors.rs
- baml_language/sdks/python/src/baml_core/proto.py
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_throws/types.baml
- baml_language/sdk_tests/harness_runner/src/lib.rs
- baml_language/sdk_tests/crates/python_pydantic2/docstrings_etc/customizable/test_raises.py
- baml_language/crates/baml_project/src/client_codegen.rs
- baml_language/sdks/python/src/baml_core/cffi/v1/baml_outbound_pb2.pyi
- baml_language/crates/bex_vm/src/errors.rs
- baml_language/crates/baml_builtins2/baml_std/baml/ns_panics/panics.baml
- baml_language/sdks/python/rust/codegen_python/src/leaf.rs
- baml_language/sdks/python/rust/bridge_python/src/runtime.rs
Contributor
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 (1)
baml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_handles.py (1)
114-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuarantee
Filehandle cleanup in tests.Line 115 opens a handle without guaranteed cleanup in one test, and cleanup is not failure-safe in the other. Wrap both tests with
try/finallysofile_closealways runs.Suggested fix
def test_open_file_returns_file_handle(temp_file): f = open_file_read(path=temp_file) - assert type(f).__name__ == "File" + try: + assert type(f).__name__ == "File" + finally: + file_close(f=f) def test_file_cursor_state_persists_across_calls(temp_file): f = open_file_read(path=temp_file) - - # Two successive reads on the *same* handle must advance the cursor — - # the second read continues where the first stopped. This is the - # load-bearing assertion: engine-side file state survives across - # separate host→engine FFI calls. - assert file_read(f=f, n=3) == "012" - assert file_read(f=f, n=3) == "345" - - # Seek back to the start and confirm the cursor actually moved. - assert file_seek(f=f, whence="start", offset=0) == 0 - assert file_read(f=f, n=2) == "01" - - # text() reads from the current cursor (now at 2) to EOF. - assert file_text(f=f) == "23456789" - - assert file_close(f=f) is None + try: + # Two successive reads on the *same* handle must advance the cursor — + # the second read continues where the first stopped. This is the + # load-bearing assertion: engine-side file state survives across + # separate host→engine FFI calls. + assert file_read(f=f, n=3) == "012" + assert file_read(f=f, n=3) == "345" + + # Seek back to the start and confirm the cursor actually moved. + assert file_seek(f=f, whence="start", offset=0) == 0 + assert file_read(f=f, n=2) == "01" + + # text() reads from the current cursor (now at 2) to EOF. + assert file_text(f=f) == "23456789" + finally: + assert file_close(f=f) is None🤖 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/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_handles.py` around lines 114 - 137, Both tests open a File handle without guaranteed cleanup; wrap the body of test_open_file_returns_file_handle and test_file_cursor_state_persists_across_calls so that the handle returned by open_file_read is closed in a finally block (call file_close(f) in finally) to ensure cleanup even on assertion failures; keep existing assertions and calls (file_read, file_seek, file_text) inside the try block and ensure file_close is only called if f is not None/was opened.
🧹 Nitpick comments (1)
baml_language/crates/bridge_ctypes/src/traceback.rs (1)
14-22: ⚡ Quick winAdd unit tests for the new traceback formatter contract.
Given this is a new host-facing formatter, add
#[cfg(test)]unit tests for empty input and multi-frame formatting to lock the wire-facing string shape.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/traceback.rs` around lines 14 - 22, Add #[cfg(test)] unit tests in the same module to lock the wire-facing string shape of format_traceback_lines: create one test that calls format_traceback_lines with an empty iterator and asserts the returned Vec<String> is empty, and another test that supplies a multi-frame iterator (e.g., frames with ("a.py", 1, "foo") and ("b.py", 2, "bar")) and asserts the Vec<String> equals the exact formatted strings like r#"File "a.py", line 1, in foo"# and r#"File "b.py", line 2, in bar"# in order; place tests in a #[cfg(test)] mod tests block and use assert_eq! for strict string matching.
🤖 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/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_handles.py`:
- Around line 83-85: The fixture teardown currently calls srv.shutdown() and
thread.join() but leaves the listening socket open; after calling srv.shutdown()
add a call to srv.server_close() before joining the thread so the HTTPServer's
socket is explicitly closed (i.e., change the teardown sequence around
srv.shutdown(), srv.server_close(), thread.join() to ensure the descriptor is
released).
In `@baml_language/sdk_tests/harness_setup/src/lib.rs`:
- Around line 264-271: The code calls fs::create_dir_all(&dst) without handling
the case where dst already exists as a file or symlink, which causes a failure;
before calling create_dir_all(&dst) check dst with std::fs::symlink_metadata or
std::fs::metadata and if it exists and is not a directory (is_file() or
file_type().is_symlink()), remove it with std::fs::remove_file(&dst) (or
remove_dir_all if you detect a directory that should be replaced), then proceed
to create_dir_all and call symlink_customizable(&src, &dst); similarly
propagate/remove errors with the existing unwrap_or_else panic pattern to
preserve current error logging.
---
Outside diff comments:
In
`@baml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_handles.py`:
- Around line 114-137: Both tests open a File handle without guaranteed cleanup;
wrap the body of test_open_file_returns_file_handle and
test_file_cursor_state_persists_across_calls so that the handle returned by
open_file_read is closed in a finally block (call file_close(f) in finally) to
ensure cleanup even on assertion failures; keep existing assertions and calls
(file_read, file_seek, file_text) inside the try block and ensure file_close is
only called if f is not None/was opened.
---
Nitpick comments:
In `@baml_language/crates/bridge_ctypes/src/traceback.rs`:
- Around line 14-22: Add #[cfg(test)] unit tests in the same module to lock the
wire-facing string shape of format_traceback_lines: create one test that calls
format_traceback_lines with an empty iterator and asserts the returned
Vec<String> is empty, and another test that supplies a multi-frame iterator
(e.g., frames with ("a.py", 1, "foo") and ("b.py", 2, "bar")) and asserts the
Vec<String> equals the exact formatted strings like r#"File "a.py", line 1, in
foo"# and r#"File "b.py", line 2, in bar"# in order; place tests in a
#[cfg(test)] mod tests block and use assert_eq! for strict string matching.
🪄 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: 7633c78a-24ea-4fbd-9a0c-0f18837daea2
📒 Files selected for processing (66)
baml_language/.config/nextest.tomlbaml_language/crates/bex_vm/src/errors.rsbaml_language/crates/bridge_cffi/src/baml_to_host.rsbaml_language/crates/bridge_ctypes/src/lib.rsbaml_language/crates/bridge_ctypes/src/traceback.rsbaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_host_callables.pybaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_raises.pybaml_language/sdk_tests/crates/python_pydantic2/setup.ps1baml_language/sdk_tests/crates/python_pydantic2/setup.shbaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_aliases.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_class_refs.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_enums.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_forward_refs.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_generics.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_handles.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_lists.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_literals.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_maps.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_media.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_optional.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_primitives.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_recursion.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_routing.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_streams.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_symbol_collisions.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_unions.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_void.pybaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/test_generic_method.pybaml_language/sdk_tests/fixtures/function_calls/baml_src/main.bamlbaml_language/sdk_tests/fixtures/function_calls/baml_src/ns_host_callable_tests/main.bamlbaml_language/sdk_tests/fixtures/function_calls/baml_src/ns_raises_test/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_class_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_enums/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_forward_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/methods.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_handles/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_ipsum/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lists/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_literals/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/cross_uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/streams.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_maps/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_media/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_optional/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_primitives/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_recursion/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_a/ns_b/ns_c/ns_d/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_buzz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_unions/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_void/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/root.bamlbaml_language/sdk_tests/harness_setup/src/lib.rsbaml_language/sdk_tests/harness_setup/src/templates/pyproject.tomlbaml_language/sdks/python/rust/bridge_python/src/media.rsbaml_language/sdks/python/rust/codegen_python/src/utils.rsbaml_language/sdks/python/setup.shbaml_language/sdks/python/src/baml_core/proto.py
💤 Files with no reviewable changes (2)
- baml_language/sdks/python/rust/codegen_python/src/utils.rs
- baml_language/sdk_tests/fixtures/function_calls/baml_src/ns_raises_test/types.baml
✅ Files skipped from review due to trivial changes (11)
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/cross_uses.baml
- baml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_void.py
- baml_language/sdk_tests/harness_setup/src/templates/pyproject.toml
- baml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/roundtrip_tests/test_lists.py
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_a/ns_b/ns_c/ns_d/uses.baml
- baml_language/sdks/python/setup.sh
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_ipsum/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_void/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_foo/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/streams.baml
- baml_language/crates/bex_vm/src/errors.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- baml_language/.config/nextest.toml
- baml_language/sdk_tests/fixtures/function_calls/baml_src/main.baml
- baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_host_callables.py
- baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_main.py
- baml_language/crates/bridge_cffi/src/baml_to_host.rs
- baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_raises.py
Comment on lines
+264
to
+271
| if src.is_dir() { | ||
| fs::create_dir_all(&dst).unwrap_or_else(|e| { | ||
| panic!( | ||
| "Failed to create {} for customizable overlay: {e}", | ||
| dst.display() | ||
| ) | ||
| }); | ||
| symlink_customizable(&src, &dst); |
Contributor
There was a problem hiding this comment.
Handle destination file↔directory collisions before recursive create_dir_all.
If dst already exists as a file/symlink (e.g., fixture layout changed from file to dir), create_dir_all(&dst) will fail and abort the build.
Suggested fix
if src.is_dir() {
+ if let Ok(meta) = dst.symlink_metadata() {
+ if meta.file_type().is_symlink() || meta.is_file() {
+ let _ = fs::remove_file(&dst);
+ }
+ }
fs::create_dir_all(&dst).unwrap_or_else(|e| {
panic!(
"Failed to create {} for customizable overlay: {e}",
dst.display()
)
});
symlink_customizable(&src, &dst);
continue;
}📝 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 src.is_dir() { | |
| fs::create_dir_all(&dst).unwrap_or_else(|e| { | |
| panic!( | |
| "Failed to create {} for customizable overlay: {e}", | |
| dst.display() | |
| ) | |
| }); | |
| symlink_customizable(&src, &dst); | |
| if src.is_dir() { | |
| if let Ok(meta) = dst.symlink_metadata() { | |
| if meta.file_type().is_symlink() || meta.is_file() { | |
| let _ = fs::remove_file(&dst); | |
| } | |
| } | |
| fs::create_dir_all(&dst).unwrap_or_else(|e| { | |
| panic!( | |
| "Failed to create {} for customizable overlay: {e}", | |
| dst.display() | |
| ) | |
| }); | |
| symlink_customizable(&src, &dst); |
🤖 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/sdk_tests/harness_setup/src/lib.rs` around lines 264 - 271, The
code calls fs::create_dir_all(&dst) without handling the case where dst already
exists as a file or symlink, which causes a failure; before calling
create_dir_all(&dst) check dst with std::fs::symlink_metadata or
std::fs::metadata and if it exists and is not a directory (is_file() or
file_type().is_symlink()), remove it with std::fs::remove_file(&dst) (or
remove_dir_all if you detect a directory that should be replaced), then proceed
to create_dir_all and call symlink_customizable(&src, &dst); similarly
propagate/remove errors with the existing unwrap_or_else panic pattern to
preserve current error logging.
New python_pydantic2 fixture function_calls covering plain (non-LLM)
expression functions:
* root main.baml: hello_world() -> "hello world" — nullary function
returning a string literal, with a smoke test in test_main.py.
* ns_host_callable_tests/ (-> baml_sdk.host_callable_tests): the
host-callable round-trip functions and tests relocated from the
standalone host_callables fixture, which is removed.
Fixture count is unchanged: host_callables folds into function_calls.
Declare `#[pyclass(module = "baml_core.baml_py")]` on the four media
PyO3 types (BamlImage/Audio/Video/Pdf) in `define_media_pyclass!`. PyO3
defaults a pyclass's `__module__` to `builtins`, which missed the
typemap reverse-map seed keyed on ("baml_core.baml_py", "BamlImage");
`py_type_to_baml_type` returned "" and the engine rejected the encoded
class ("Unknown class ``"). Reporting the honest module makes the seed
match, so media values round-trip on the encode path.
Removed the five `test_media.py::test_round_trip_*` strict-xfail markers;
they now pass. roundtrip_tests/ -> 99 passed, 2 skipped;
sdk_test_python_pydantic2 -> 13 passed.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
This was referenced May 28, 2026
Closed
Closed
This branch was previously deployed
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
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
New Features
Raises:information.Bug Fixes
Tests
Documentation