ItemTree salsa firewalls#4064
Merged
Merged
Conversation
The AST's `TypeExpr` holds spans which clobbers the salsa cache for whitespace changes. Based on rust-analyzer, `TypeRef` is the HIR's version, getting an item-scoped index that should be much more stable. The source map for the type refs is then a separate query. In this commit, `TypeRef` is currently unused.
This separates the builder logic from the immutable `ItemTree` so the index counters should not clobber salsa Also record mappings between items and scopes instead of using span comparisons
Nothing upstream should be using the HIR's item tree as it excludes synthetic items and thus has different indexing. All the firewall queries have been moved there for the downstream consumers.
And some other conversion to the firewalled queries
We should now lower the salsa-stable type refs instead of type exprs (`lower_type_expr` is now a shim)
Span changes with whitespace so is not a cache-friendly strategy (and it can be ambiguous for synthetic items). For this reason we also change several points where we were looking up by span to instead use the much more stable id.
Acts as a firewall for llm function metadata that does not require full information or span data
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
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:
📝 WalkthroughWalkthroughCompiler2 now builds span-free HIR and PPIR data with parallel source maps, stable item and scope ownership indices, deterministic enumeration, and TypeRef-based lowering. Compiler consumers, CLI tooling, project metadata, LSP actions, and incremental tests use the canonical PPIR query layer. ChangesCompiler2 canonical data pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant HIR as HIR ItemTreeBuilder
participant PPIR as PPIR item_data
participant TIR as TIR lower_type_ref
participant Tooling as CLI/LSP/Project tooling
HIR->>PPIR: Build canonical item locations and semantic data
PPIR->>TIR: Provide TypeRef arenas and owner/scope queries
PPIR->>Tooling: Provide deterministic items, metadata, and source maps
TIR->>Tooling: Produce typed results and diagnostics
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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: 4
🧹 Nitpick comments (1)
baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs (1)
1002-1058: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftKeep canonical item consumers behind PPIR item firewalls.
Exact owner IDs are available, but these paths subsequently read the complete
file_item_tree, retaining file-wide invalidation.
baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs#L1002-L1058: gather owner and function bounds through item-specific PPIR queries.baml_language/crates/baml_compiler2_tir/src/inference.rs#L810-L824: remove the full tree fromGenericLookupContext.baml_language/crates/baml_compiler2_tir/src/inference.rs#L1712-L2165: fetch function, impl, and interface metadata through firewall queries.baml_language/crates/baml_compiler2_tir/src/inference.rs#L2328-L2560: fetch ancestor function/class/interface data through their item queries.baml_language/crates/baml_compiler2_tir/src/inference.rs#L2900-L2908: fetch type-alias data through its item query.🤖 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/baml_compiler2_tir/src/lower_type_expr.rs` around lines 1002 - 1058, Route all canonical item metadata access through PPIR item-specific firewall queries instead of the complete file_item_tree. In lower_type_expr.rs:1002-1058, update the owner and function-bound lookup around method_owner and function_id; in inference.rs:810-824, remove the full tree from GenericLookupContext; in inference.rs:1712-2165 and 2328-2560, use item queries for function, impl, interface, ancestor, and class metadata; in inference.rs:2900-2908, use the type-alias item query.
🤖 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_compiler2_hir/src/item_tree.rs`:
- Around line 46-52: Remove source-position data from ItemTree semantic equality
by separating all TextRange, AstSourceMap, spans, name spans, and span-bearing
AST values into parallel source-map structures. In
baml_language/crates/baml_compiler2_hir/src/item_tree/classes.rs:14-46, use
span-free type references; in enums.rs:25-26, move enum declaration spans; in
interfaces.rs:18-114, separate interface, signature, impl, and field-link
positions; in lets.rs:10-13, keep AstSourceMap, span, and name_span outside Let;
in template_strings.rs:9-14, separate parameter/declaration spans; and in
type_aliases.rs:8-11, store span-free TypeRef with parallel source-map data.
Update item_tree.rs:46-52 and related constructors/accessors so ItemTree
equality depends only on semantic values.
In `@baml_language/crates/baml_compiler2_hir/src/item_tree/builder.rs`:
- Around line 174-179: Update record_method_owner to enforce duplicate ownership
checks in release builds, using the entry API or an unconditional assertion
before replacing an existing owner. Preserve the existing diagnostic showing
both owners, and ensure duplicate method IDs cannot silently overwrite the
original entry.
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 3362-3383: Update lower_signature_runtime_ty to derive self_ty
from method_owners instead of always creating rigid TypeVar("Self"): use the
concrete class/impl receiver for non-interface methods, bounded rigid Self for
interface methods, and None for free functions. Mirror the owner-specific logic
used by the emitter around method_owners so class and impl Self references lower
concretely without adding a Self slot to free-function frames.
In `@baml_language/crates/baml_compiler2_ppir/src/item_data/enumeration.rs`:
- Around line 22-34: Update the sorting in the file_items macro’s generated
functions to use a deterministic secondary key when item.span.start() values are
equal, such as the item’s stable name and ID. Preserve source-offset ordering
while ensuring synthetic offset-zero items no longer depend on FxHashMap
iteration order.
---
Nitpick comments:
In `@baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs`:
- Around line 1002-1058: Route all canonical item metadata access through PPIR
item-specific firewall queries instead of the complete file_item_tree. In
lower_type_expr.rs:1002-1058, update the owner and function-bound lookup around
method_owner and function_id; in inference.rs:810-824, remove the full tree from
GenericLookupContext; in inference.rs:1712-2165 and 2328-2560, use item queries
for function, impl, interface, ancestor, and class metadata; in
inference.rs:2900-2908, use the type-alias item query.
🪄 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: 85099d70-ae69-463f-b40e-e087ba616214
📒 Files selected for processing (51)
baml_language/ARCHITECTURE.mdbaml_language/crates/baml_cli/src/test_command.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/item_tree.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/builder.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/classes.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/clients.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/common.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/enums.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/functions.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/interfaces.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/lets.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/retry_policies.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/source_map.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/template_strings.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/test_items.rsbaml_language/crates/baml_compiler2_hir/src/item_tree/type_aliases.rsbaml_language/crates/baml_compiler2_hir/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/scope.rsbaml_language/crates/baml_compiler2_hir/src/semantic_index.rsbaml_language/crates/baml_compiler2_hir/src/signature.rsbaml_language/crates/baml_compiler2_hir/src/type_ref.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_ppir/src/item_data.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/classes.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/clients.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/common.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/enumeration.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/enums.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/functions.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/impls.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/interfaces.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/lets.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/retry_policies.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/scopes.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/template_strings.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/test_items.rsbaml_language/crates/baml_compiler2_ppir/src/item_data/type_aliases.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/callable.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_lsp2_actions/src/annotations.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/definition.rsbaml_language/crates/baml_project/src/db.rsbaml_language/crates/baml_project/src/symbols.rsbaml_language/crates/baml_tests/src/incremental/scenarios.rs
Binary size checks passed✅ 7 passed
Generated by |
…-12681713d594ad95ea52ece395acb33ed859a3f0' into kai/hir-salsa
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/README.md (1)
155-161: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify that Web fixtures reuse the TypeScript customizable corpus.
Step 2 says every generator target gets its own
customizable/directory, contradicting the documented Web layout. State thattypescript_webconsumescrates/typescript/<fixture>/customizableand needs no parallel directory.🤖 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/README.md` around lines 155 - 161, Update the fixture setup instructions around step 2 to clarify that the typescript_web target reuses the customizable tests under crates/typescript/<name>/customizable. State that no parallel customizable directory is needed under the web crate, while preserving the existing directory guidance for other generator targets.
🤖 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/typescript/function_calls/customizable/cancellation.test.ts`:
- Around line 142-161: Update the test named “surfaces a reused call context as
AbortError with BAML reason” to invoke callFunction a second time using the
already-aborted ctx and assert that this reused-context call rejects with
expectBamlCancelledReason. Preserve the existing first-call cancellation
assertions and test intent.
In `@baml_language/sdk_tests/README.md`:
- Around line 41-44: Update the test command sequence in the SDK README to run
the sdk_test_typescript_web setup before invoking any Vitest commands under the
typescript_web generated fixture. Keep the existing sdk_test_typescript setup
and test commands unchanged, and place the Web initialization before the Web
Vitest configurations.
---
Outside diff comments:
In `@baml_language/sdk_tests/README.md`:
- Around line 155-161: Update the fixture setup instructions around step 2 to
clarify that the typescript_web target reuses the customizable tests under
crates/typescript/<name>/customizable. State that no parallel customizable
directory is needed under the web crate, while preserving the existing directory
guidance for other generator targets.
🪄 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: 8a38ad39-1a7e-43eb-92cd-6429b7f901dc
⛔ Files ignored due to path filters (18)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/bigints.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/floats.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/ints.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/catch_arm_return/baml_tests__compiles__catch_arm_return__01_lexer__catch_arm_return.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/numeric_literal_method_call/baml_tests__compiles__numeric_literal_method_call__01_lexer__numeric_literal_method_call.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/invalid_numeric_literals/baml_tests__diagnostic_errors__invalid_numeric_literals__01_lexer__negative.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/invalid_numeric_literals/baml_tests__diagnostic_errors__invalid_numeric_literals__01_lexer__positive.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/invalid_numeric_literals/baml_tests__diagnostic_errors__invalid_numeric_literals__02_parser__negative.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/invalid_numeric_literals/baml_tests__diagnostic_errors__invalid_numeric_literals__02_parser__positive.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/invalid_numeric_literals/baml_tests__diagnostic_errors__invalid_numeric_literals__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/invalid_numeric_literals/baml_tests__diagnostic_errors__invalid_numeric_literals__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/invalid_numeric_literals/baml_tests__diagnostic_errors__invalid_numeric_literals__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/invalid_numeric_literals/baml_tests__diagnostic_errors__invalid_numeric_literals__10_formatter__negative.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/invalid_numeric_literals/baml_tests__diagnostic_errors__invalid_numeric_literals__10_formatter__positive.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/json_static_method_arity/baml_tests__diagnostic_errors__json_static_method_arity__01_lexer__json_static_method_arity.snapis excluded by!**/*.snapbaml_language/sdks/typescript/bridge_typescript_web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (66)
.github/actions/setup-mise/action.yml.github/workflows/build2-web-sdk.reusable.yaml.github/workflows/cargo-tests.reusable.yaml.github/workflows/wasm-pack-tests.reusable.yaml.github/workflows/webview-tests.reusable.yamlbaml_language/.config/nextest.tomlbaml_language/Cargo.tomlbaml_language/crates/baml_base/src/lib.rsbaml_language/crates/baml_base/src/num_lit.rsbaml_language/crates/baml_builtins2/baml_std/baml/bigint.bamlbaml_language/crates/baml_builtins2/baml_std/baml/int.bamlbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_config_item.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_ast/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_ast/src/lowering_diagnostic.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler_diagnostics/src/diagnostic.rsbaml_language/crates/baml_compiler_lexer/src/tokens.rsbaml_language/crates/baml_compiler_syntax/src/ast.rsbaml_language/crates/baml_compiler_syntax/src/tests.rsbaml_language/crates/baml_tests/baml_src/ns_bigints/bigints.bamlbaml_language/crates/baml_tests/baml_src/ns_floats/floats.bamlbaml_language/crates/baml_tests/baml_src/ns_ints/ints.bamlbaml_language/crates/baml_tests/projects/diagnostic_errors/invalid_numeric_literals/negative.bamlbaml_language/crates/baml_tests/projects/diagnostic_errors/invalid_numeric_literals/positive.bamlbaml_language/sdk_tests/DEVELOPMENT.mdbaml_language/sdk_tests/README.mdbaml_language/sdk_tests/crates/typescript/build.rsbaml_language/sdk_tests/crates/typescript/docstrings_etc/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript/function_calls/customizable/cancellation.node.test.tsbaml_language/sdk_tests/crates/typescript/function_calls/customizable/cancellation.test.tsbaml_language/sdk_tests/crates/typescript/function_calls/customizable/runtime.node.test.tsbaml_language/sdk_tests/crates/typescript/function_calls/customizable/runtime.test.tsbaml_language/sdk_tests/crates/typescript/function_calls/customizable/stdlib_entrypoints.node.test.tsbaml_language/sdk_tests/crates/typescript/function_calls/customizable/stdlib_entrypoints.test.tsbaml_language/sdk_tests/crates/typescript/llm_functions/customizable/replay_harness.tsbaml_language/sdk_tests/crates/typescript/llm_functions/customizable/streaming_e2e.test.tsbaml_language/sdk_tests/crates/typescript/setup.ps1baml_language/sdk_tests/crates/typescript/setup.shbaml_language/sdk_tests/crates/typescript/src/lib.rsbaml_language/sdk_tests/crates/typescript/type_shapes/customizable/roundtrip_handles.node.test.tsbaml_language/sdk_tests/crates/typescript/type_shapes/customizable/roundtrip_handles.test.tsbaml_language/sdk_tests/crates/typescript/type_shapes/customizable/roundtrip_media.node.test.tsbaml_language/sdk_tests/crates/typescript/type_shapes/customizable/roundtrip_media.test.tsbaml_language/sdk_tests/crates/typescript_web/Cargo.tomlbaml_language/sdk_tests/crates/typescript_web/build.rsbaml_language/sdk_tests/crates/typescript_web/setup.ps1baml_language/sdk_tests/crates/typescript_web/setup.shbaml_language/sdk_tests/crates/typescript_web/src/lib.rsbaml_language/sdk_tests/harness_runner/Cargo.tomlbaml_language/sdk_tests/harness_runner/src/lib.rsbaml_language/sdk_tests/harness_setup/src/lib.rsbaml_language/sdk_tests/harness_setup/src/templates/package_node.jsonbaml_language/sdk_tests/harness_setup/src/templates/package_web.jsonbaml_language/sdk_tests/harness_setup/src/templates/test_runtime.tsbaml_language/sdk_tests/harness_setup/src/templates/tsconfig_web.jsonbaml_language/sdk_tests/harness_setup/src/templates/vitest_node.config.tsbaml_language/sdk_tests/harness_setup/src/templates/vitest_web.config.tsbaml_language/sdk_tests/harness_setup/src/templates/vitest_workers.config.tsbaml_language/sdk_tests/harness_setup/src/typescript.rsbaml_language/sdk_tests/harness_setup/src/typescript_web.rsbaml_language/sdks/typescript/bridge_typescript_web/package.jsonmise.toml
💤 Files with no reviewable changes (5)
- baml_language/sdk_tests/crates/typescript/function_calls/customizable/runtime.node.test.ts
- baml_language/sdk_tests/crates/typescript/type_shapes/customizable/roundtrip_handles.node.test.ts
- baml_language/sdk_tests/crates/typescript/function_calls/customizable/cancellation.node.test.ts
- baml_language/sdk_tests/crates/typescript/type_shapes/customizable/roundtrip_media.node.test.ts
- baml_language/sdk_tests/crates/typescript/function_calls/customizable/stdlib_entrypoints.node.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/crates/baml_compiler2_tir/src/builder.rs
2kai2kai2
enabled auto-merge
July 17, 2026 00:04
…-1d41d643ef32c187a87d70c1460f700f8db88aa4' into kai/hir-salsa
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
baml_language/sdks/rust/bridge_rust/src/test_support.rs-17-18 (1)
17-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch the loader’s non-empty environment semantics in both test helpers.
baml_language/sdks/rust/bridge_rust/src/test_support.rs#L17-L18: treat an emptyBAML_LIBRARY_PATHas unset.baml_language/sdks/rust/bridge_rust/tests/common/mod.rs#L16-L17: apply the same non-empty check to the integration helper.🤖 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/rust/bridge_rust/src/test_support.rs` around lines 17 - 18, Treat BAML_LIBRARY_PATH as configured only when its value is non-empty in both test helpers: update the environment check in baml_language/sdks/rust/bridge_rust/src/test_support.rs lines 17-18 and apply the same change in baml_language/sdks/rust/bridge_rust/tests/common/mod.rs lines 16-17, preserving the existing early-return behavior for non-empty values.baml_language/sdk_tests/crates/rust/function_calls/customizable/test_generic_inference.rs-23-28 (1)
23-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep host-callable coverage out of the generics-only gate.
This module imports and calls
apply, although host-callable support has not landed. Flippingtest_generic_inference.rsfromGate::Later("needs generics")will therefore remain blocked by an unrelated capability. Move this test/import totest_host_callables.rsor give it a separate gate.Also applies to: 719-738
🤖 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/rust/function_calls/customizable/test_generic_inference.rs` around lines 23 - 28, Move the apply-related test coverage and its apply import out of test_generic_inference.rs into test_host_callables.rs, or place it under a separate host-callables gate. Keep the generics-only tests and imports independent of host-callable support so enabling the “needs generics” gate is not blocked by apply.baml_language/sdk_tests/crates/rust/function_calls/customizable/test_errors.rs-280-287 (1)
280-287: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not let the unimplemented clean-exit probe report success.
Once this module is enabled, this empty test passes without validating either
exit(0)orexit(7). Mark it ignored until the subprocess harness exists, or implement the harness now.Minimal interim fix
#[test] +#[ignore = "requires a subprocess harness to validate process exit codes"] fn test_clean_exit_terminates_process_with_code() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/crates/rust/function_calls/customizable/test_errors.rs` around lines 280 - 287, Prevent test_clean_exit_terminates_process_with_code from reporting success without assertions by marking it ignored until a subprocess harness is available. Preserve the existing test name and explanatory divergence comment, and do not implement unrelated changes.baml_language/sdk_tests/crates/rust/function_calls/customizable/test_host_callables.rs-208-215 (1)
208-215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the struct pattern for
Error::Thrown.baml_bridge::Error<E>definesThrown { value, trace }, soError::Thrown(decoded)won't match here. Change it toError::Thrown { value: decoded, .. }.🤖 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/rust/function_calls/customizable/test_host_callables.rs` around lines 208 - 215, Update the error match in the typed-throw propagation test to use the struct pattern for baml_bridge::Error::Thrown, binding its value field to decoded and ignoring trace. Preserve the existing panic behavior for other error variants.baml_language/sdk_tests/harness_setup/src/rust.rs-73-75 (1)
73-75: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd Tokio
timeto the generated dev-dependencies.
baml_language/sdk_tests/crates/rust/function_calls/customizable/test_cancellation.rsandtest_errors.rsusetokio::time::sleep/timeout, so the fixture manifest needstimein addition tortandmacros.🤖 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/rust.rs` around lines 73 - 75, Update the MANIFEST_EXTRA generated dev-dependencies declaration to include Tokio’s time feature alongside the existing rt and macros features, so test_cancellation.rs and test_errors.rs can use tokio::time::sleep and timeout.baml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_aliases.rs-13-45 (1)
13-45: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRepresent unsupported roundtrips as skipped, not passing.
baml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_aliases.rs#L13-L45: mark both recursive-alias placeholders ignored or remove their test attributes.baml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_forward_refs.rs#L17-L49: do the same for both recursive-union placeholders.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/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_aliases.rs` around lines 13 - 45, Mark both recursive-alias placeholder tests, test_round_trip_rec_list and test_round_trip_alias_container, as ignored or remove their test attributes in baml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_aliases.rs:13-45. Apply the same treatment to both recursive-union placeholder tests in baml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_forward_refs.rs:17-49, so unsupported roundtrips are skipped rather than reported as passing.Source: Coding guidelines
🧹 Nitpick comments (1)
scripts/baml-release-manifests (1)
13-29: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAllow successful experimental artifacts in generated manifests.
TARGETSexcludes experimental toolchain targets, whilecollect_artifacts()rejects every artifact outsideTARGETS. Once an experimental target is added, a successful best-effort build will therefore fail manifest generation as an “extra.”Track both supported and required targets: check missing artifacts against required targets and extras against all supported targets.
🤖 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 `@scripts/baml-release-manifests` around lines 13 - 29, Update _toolchain_required_targets and the manifest validation flow to track both required and all supported toolchain targets, including experimental entries from release/platforms.json. Keep missing-artifact checks against required targets, but validate extras against the complete supported-target set so successful experimental artifacts are accepted.
🤖 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/.config/nextest.toml`:
- Around line 14-16: Remove the duplicated TOML table declarations in the
nextest configuration, including repeated [scripts.setup.*] headers and the
extra [[profile.default.overrides]] declaration around the profile overrides.
Consolidate each setup table and override entries under a single declaration so
nextest parses the configuration without empty or redefined tables.
In `@baml_language/crates/baml_release/src/platforms.rs`:
- Around line 20-24: Add #[serde(deny_unknown_fields)] to the Platforms contract
struct and the related platform contract structs used for deserialization, then
add a test that supplies an unknown or misspelled key and asserts
deserialization fails. Preserve valid platform-contract parsing and existing
field defaults.
In `@baml_language/crates/baml_tests/tests/env.rs`:
- Line 94: Guard or serialize the process-global environment mutations in the
tests at baml_language/crates/baml_tests/tests/env.rs lines 94, 124, and 153.
Ensure each OPENAI_API_KEY update restores its prior value after the test, or
otherwise prevents concurrent execution across all three sites; apply the same
protection consistently to each mutation.
In `@baml_language/crates/bex_engine/src/conversion.rs`:
- Around line 73-86: Update the VM-to-external conversion walk in conversion.rs
to track visited HeapPtr identities across recursive array, map, and instance
conversions, passing the visited state through each recursive call. Detect
back-edges and return an EngineError instead of recursing further, preserving
normal conversion for acyclic values. Add a unit test covering a cyclic value
and asserting the conversion returns an error.
In `@baml_language/crates/sys_llm/src/auth_request/mod.rs`:
- Around line 119-129: Change resolve_api_key to return Result<Option<String>,
BuildRequestError>, preserving the configured-key and
absent-environment-variable cases while propagating RuntimeIoError instead of
converting it to None. Update auth_openai and auth_anthropic to propagate this
result through their authentication request flows and adjust callers to handle
the new error type.
In
`@baml_language/sdk_tests/crates/rust/llm_functions/customizable/replay_harness.rs`:
- Around line 119-126: The Drop shutdown path around post_shutdown and
thread.join must not block indefinitely. Poll JoinHandle::is_finished() until a
bounded deadline, joining only when the thread completes; after timeout, detach
the handle and report the forced shutdown so the test process can continue.
- Around line 103-132: Centralize environment isolation across both sites: in
baml_language/sdk_tests/crates/rust/llm_functions/customizable/replay_harness.rs:103-132,
have RunningServer acquire and retain the shared test lock for its lifetime,
snapshot existing BAML_REPLAY_BASE_URL and BAML_REPLAY_API_KEY values, and
restore them on Drop instead of removing them unconditionally; in
baml_language/sdk_tests/crates/rust/llm_functions/customizable/test_main.rs:196-258,
use the same scoped lock-and-restore mechanism for provider environment keys
rather than permanent set_var calls.
In `@baml_language/sdks/rust/bridge_rust/src/capi.rs`:
- Around line 159-165: Update the ABI loading logic around get_api_v1 so it
reads a raw, minimal prefix containing abi_version and struct_size before
constructing any reference. Validate the returned pointer is non-null and the
prefix is large enough for the required fields, then reject incompatible
versions or undersized tables; only after these checks cast the pointer to
&BamlApiV1 and retain the existing full-table validation behavior.
- Around line 95-104: Update load so initialization never performs synchronous
library resolution or downloading on a current-thread Tokio runtime. Preserve
the existing block_in_place path for multi-thread runtimes, and route
current-thread async callers through spawn_blocking or enforce an explicit
preload before api() dispatch, while retaining synchronous loading outside
Tokio.
In `@baml_language/sdks/rust/bridge_rust/src/completion.rs`:
- Around line 182-190: Update the panic handling around the completion callback
to record a failed state in State, then wake or notify the associated receiver
so both wait paths return an internal error instead of remaining pending.
Preserve the existing stderr logging and C-boundary panic containment, and use
the receiver’s established completion/notification mechanism.
In `@baml_language/sdks/rust/bridge_rust/src/loader/download.rs`:
- Around line 36-48: Update the checksum handling around fetch_checksum and the
subsequent download/install flow to fail closed when the checksum is missing,
malformed, or unreachable. Require a trusted or pinned checksum before
permitting the artifact to be downloaded and installed; remove the path that
returns None and proceeds without verification, including the corresponding
logic around the additionally referenced download flow.
- Around line 134-146: Update the rename-failure path in the download flow
around copy_file so installation remains atomic. Do not copy directly to
dest_path; instead write the fallback to a temporary destination and publish it
with an atomic rename, or otherwise use locking that prevents readers from
observing partial files. Preserve the existing LoaderError::DownloadFailed
reporting and success logging.
In `@baml_language/sdks/rust/bridge_rust/src/loader/mod.rs`:
- Around line 99-117: Synchronize set_shared_library_path with the beginning of
engine loading: update the shared state guarded by EXPLICIT_PATH so loading
marks configuration as started before snapshotting the path, and have
set_shared_library_path reject and warn when that state is active even if
engine_loaded() is still false. Update the relevant loader initialization flow
and add a concurrency regression test covering a setter racing with the initial
load.
In `@baml_language/sdks/rust/sdkgen_rust/src/analyze.rs`:
- Around line 447-502: Update compute_renames to choose each colliding child
segment by repeatedly appending "_" until the resulting name is unused among
both sibling namespace paths in all_paths and emitted type names in types_in for
the parent; preserve ancestor renames and apply the selected name to
descendants. Add a unit test covering a type Foo with namespaces Foo and Foo_,
asserting the renamed namespaces remain distinct.
In `@baml_language/sdks/rust/sdkgen_rust/src/emit/function.rs`:
- Around line 87-104: Reserve and de-conflict generator-owned identifiers before
emitting code: update the function emission logic around sync_name and
async_name so each leaf handles collisions between foo and foo_async, and update
the root generation logic in baml_language/sdks/rust/sdkgen_rust/src/lib.rs at
lines 351-370 to reserve _inlinedbaml, _runtime, and the root init export. Add
regression coverage for both function-name collisions and root symbols using
each reserved internal name.
In `@baml_language/sdks/rust/sdkgen_rust/src/unions.rs`:
- Around line 173-214: Extend shape_error to detect overlapping union arms after
wire encoding or Rust lowering, not only duplicate variant names and
string-literal conflicts. Ensure container arms such as int[] | string[] with
shared empty-list representations and transparent aliases such as AliasToString
| string are rejected before enum generation, preventing ambiguous decoding and
conflicting From<String> implementations. Keep analyze and collect using this
shared validation, and add regression tests covering both container overlap and
alias/string overlap.
In `@scripts/baml-language-version`:
- Around line 354-363: Update the registry_versions validation in the
data-loading flow to require every required entry from PLAN_REGISTRIES to be a
non-empty JSON string, rejecting nulls, booleans, numbers, and structured
values. Remove the str() coercion when constructing registry_versions, and
perform validation before creating ReleasePlan while preserving the existing
missing-registry checks.
---
Minor comments:
In
`@baml_language/sdk_tests/crates/rust/function_calls/customizable/test_errors.rs`:
- Around line 280-287: Prevent test_clean_exit_terminates_process_with_code from
reporting success without assertions by marking it ignored until a subprocess
harness is available. Preserve the existing test name and explanatory divergence
comment, and do not implement unrelated changes.
In
`@baml_language/sdk_tests/crates/rust/function_calls/customizable/test_generic_inference.rs`:
- Around line 23-28: Move the apply-related test coverage and its apply import
out of test_generic_inference.rs into test_host_callables.rs, or place it under
a separate host-callables gate. Keep the generics-only tests and imports
independent of host-callable support so enabling the “needs generics” gate is
not blocked by apply.
In
`@baml_language/sdk_tests/crates/rust/function_calls/customizable/test_host_callables.rs`:
- Around line 208-215: Update the error match in the typed-throw propagation
test to use the struct pattern for baml_bridge::Error::Thrown, binding its value
field to decoded and ignoring trace. Preserve the existing panic behavior for
other error variants.
In
`@baml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_aliases.rs`:
- Around line 13-45: Mark both recursive-alias placeholder tests,
test_round_trip_rec_list and test_round_trip_alias_container, as ignored or
remove their test attributes in
baml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_aliases.rs:13-45.
Apply the same treatment to both recursive-union placeholder tests in
baml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_forward_refs.rs:17-49,
so unsupported roundtrips are skipped rather than reported as passing.
In `@baml_language/sdk_tests/harness_setup/src/rust.rs`:
- Around line 73-75: Update the MANIFEST_EXTRA generated dev-dependencies
declaration to include Tokio’s time feature alongside the existing rt and macros
features, so test_cancellation.rs and test_errors.rs can use tokio::time::sleep
and timeout.
In `@baml_language/sdks/rust/bridge_rust/src/test_support.rs`:
- Around line 17-18: Treat BAML_LIBRARY_PATH as configured only when its value
is non-empty in both test helpers: update the environment check in
baml_language/sdks/rust/bridge_rust/src/test_support.rs lines 17-18 and apply
the same change in baml_language/sdks/rust/bridge_rust/tests/common/mod.rs lines
16-17, preserving the existing early-return behavior for non-empty values.
---
Nitpick comments:
In `@scripts/baml-release-manifests`:
- Around line 13-29: Update _toolchain_required_targets and the manifest
validation flow to track both required and all supported toolchain targets,
including experimental entries from release/platforms.json. Keep
missing-artifact checks against required targets, but validate extras against
the complete supported-target set so successful experimental artifacts are
accepted.
🪄 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: 83e62d77-f175-4fca-917a-81a8580e57ee
⛔ Files ignored due to path filters (24)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_tests/snapshots/compiles/comment_in_type/baml_tests__compiles__comment_in_type__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/comment_in_type/baml_tests__compiles__comment_in_type__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/comment_in_type/baml_tests__compiles__comment_in_type__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/comment_in_type/baml_tests__compiles__comment_in_type__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/config_dictionary/baml_tests__compiles__config_dictionary__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/config_dictionary/baml_tests__compiles__config_dictionary__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/config_dictionary/baml_tests__compiles__config_dictionary__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/config_dictionary/baml_tests__compiles__config_dictionary__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/retry_policy/baml_tests__compiles__retry_policy__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/retry_policy/baml_tests__compiles__retry_policy__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/retry_policy/baml_tests__compiles__retry_policy__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/retry_policy/baml_tests__compiles__retry_policy__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/client_option_types/baml_tests__diagnostic_errors__client_option_types__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/client_option_types/baml_tests__diagnostic_errors__client_option_types__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/control_flow/baml_tests__diagnostic_errors__control_flow__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/control_flow/baml_tests__diagnostic_errors__control_flow__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/format_checks/baml_tests__diagnostic_errors__format_checks__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/format_checks/baml_tests__diagnostic_errors__format_checks__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__stream_expansion__stream_companion_preserves_generic_args_for_llm_return_type.snapis excluded by!**/*.snap
📒 Files selected for processing (124)
.github/actions/setup-mise/action.yml.github/actions/setup-musl-cross/action.yml.github/workflows/build2-bridge-cffi.reusable.yaml.github/workflows/build2-python-sdk.reusable.yaml.github/workflows/build2-rust-sdk.reusable.yaml.github/workflows/cargo-tests.reusable.yaml.github/workflows/ci.yaml.github/workflows/release-baml-language.yml.github/workflows/verify-rust-sdk.reusable.yamlbaml_language/.cargo/size-gate.tomlbaml_language/.ci/size-gate/aarch64-apple-darwin.tomlbaml_language/.ci/size-gate/x86_64-pc-windows-msvc.tomlbaml_language/.ci/size-gate/x86_64-unknown-linux-gnu.tomlbaml_language/.config/nextest.tomlbaml_language/Cargo.tomlbaml_language/crates/baml_base/src/client_options.rsbaml_language/crates/baml_base/src/lib.rsbaml_language/crates/baml_cli/Cargo.tomlbaml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_codegen_types/src/generator_fields.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lowering_diagnostic.rsbaml_language/crates/baml_release/src/lib.rsbaml_language/crates/baml_release/src/manifest.rsbaml_language/crates/baml_release/src/platforms.rsbaml_language/crates/baml_tests/tests/env.rsbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bridge_ctypes/README.mdbaml_language/crates/bridge_ctypes/build.rsbaml_language/crates/sys_llm/src/auth_request/mod.rsbaml_language/crates/sys_llm/src/baml_std.rsbaml_language/crates/sys_llm/src/build_request/openai/chat_completions.rsbaml_language/crates/sys_llm/src/provider.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/sdk_tests/DEVELOPMENT.mdbaml_language/sdk_tests/README.mdbaml_language/sdk_tests/crates/rust/Cargo.tomlbaml_language/sdk_tests/crates/rust/build.rsbaml_language/sdk_tests/crates/rust/docstrings_etc/customizable/test_main.rsbaml_language/sdk_tests/crates/rust/function_calls/customizable/optional_args_static.rsbaml_language/sdk_tests/crates/rust/function_calls/customizable/test_cancellation.rsbaml_language/sdk_tests/crates/rust/function_calls/customizable/test_errors.rsbaml_language/sdk_tests/crates/rust/function_calls/customizable/test_generic_calls.rsbaml_language/sdk_tests/crates/rust/function_calls/customizable/test_generic_inference.rsbaml_language/sdk_tests/crates/rust/function_calls/customizable/test_host_callables.rsbaml_language/sdk_tests/crates/rust/function_calls/customizable/test_main.rsbaml_language/sdk_tests/crates/rust/function_calls/customizable/test_methods_on_classes.rsbaml_language/sdk_tests/crates/rust/function_calls/customizable/test_optional_args.rsbaml_language/sdk_tests/crates/rust/function_calls/customizable/test_raises.rsbaml_language/sdk_tests/crates/rust/function_calls/customizable/test_stdlib_entrypoints.rsbaml_language/sdk_tests/crates/rust/llm_functions/customizable/replay_harness.rsbaml_language/sdk_tests/crates/rust/llm_functions/customizable/test_main.rsbaml_language/sdk_tests/crates/rust/llm_functions/customizable/test_streaming_e2e.rsbaml_language/sdk_tests/crates/rust/setup.ps1baml_language/sdk_tests/crates/rust/setup.shbaml_language/sdk_tests/crates/rust/src/lib.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_aliases.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_class_refs.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_enums.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_forward_refs.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_generics.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_handles.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_lists.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_literals.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_maps.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_media.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_optional.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_primitives.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_recursion.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_routing.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_streams.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_symbol_collisions.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_unions.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/roundtrip_tests/test_void.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/test_complex_models.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/test_generic.rsbaml_language/sdk_tests/crates/rust/type_shapes/customizable/test_main.rsbaml_language/sdk_tests/harness_runner/src/lib.rsbaml_language/sdk_tests/harness_setup/Cargo.tomlbaml_language/sdk_tests/harness_setup/src/lib.rsbaml_language/sdk_tests/harness_setup/src/rust.rsbaml_language/sdks/rust/bridge_rust/Cargo.tomlbaml_language/sdks/rust/bridge_rust/README.mdbaml_language/sdks/rust/bridge_rust/build-engine.ps1baml_language/sdks/rust/bridge_rust/build-engine.shbaml_language/sdks/rust/bridge_rust/src/baml_value.rsbaml_language/sdks/rust/bridge_rust/src/capi.rsbaml_language/sdks/rust/bridge_rust/src/completion.rsbaml_language/sdks/rust/bridge_rust/src/decode.rsbaml_language/sdks/rust/bridge_rust/src/encode.rsbaml_language/sdks/rust/bridge_rust/src/error.rsbaml_language/sdks/rust/bridge_rust/src/lib.rsbaml_language/sdks/rust/bridge_rust/src/loader/download.rsbaml_language/sdks/rust/bridge_rust/src/loader/log.rsbaml_language/sdks/rust/bridge_rust/src/loader/mod.rsbaml_language/sdks/rust/bridge_rust/src/runtime.rsbaml_language/sdks/rust/bridge_rust/src/test_support.rsbaml_language/sdks/rust/bridge_rust/src/version.rsbaml_language/sdks/rust/bridge_rust/src/wire/baml_bridge.cffi.v1.rsbaml_language/sdks/rust/bridge_rust/src/wire/mod.rsbaml_language/sdks/rust/bridge_rust/tests/common/mod.rsbaml_language/sdks/rust/bridge_rust/tests/conversions.rsbaml_language/sdks/rust/bridge_rust/tests/live_engine.rsbaml_language/sdks/rust/sdkgen_rust/Cargo.tomlbaml_language/sdks/rust/sdkgen_rust/src/analyze.rsbaml_language/sdks/rust/sdkgen_rust/src/emit/class.rsbaml_language/sdks/rust/sdkgen_rust/src/emit/enum_.rsbaml_language/sdks/rust/sdkgen_rust/src/emit/function.rsbaml_language/sdks/rust/sdkgen_rust/src/emit/mod.rsbaml_language/sdks/rust/sdkgen_rust/src/emit/type_alias.rsbaml_language/sdks/rust/sdkgen_rust/src/emit/union.rsbaml_language/sdks/rust/sdkgen_rust/src/idents.rsbaml_language/sdks/rust/sdkgen_rust/src/lib.rsbaml_language/sdks/rust/sdkgen_rust/src/routing.rsbaml_language/sdks/rust/sdkgen_rust/src/translate_ty.rsbaml_language/sdks/rust/sdkgen_rust/src/unions.rsbaml_language/sdks/rust/verify/baml.tomlbaml_language/sdks/rust/verify/baml_src/main.bamlbaml_language/sdks/rust/verify/consumer_main.rsbaml_language/stow.tomlrelease/platforms.jsonscripts/baml-language-versionscripts/baml-release-manifests
🚧 Files skipped from review as they are similar to previous changes (2)
- baml_language/sdk_tests/harness_setup/src/lib.rs
- baml_language/sdk_tests/DEVELOPMENT.md
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (17)
baml_language/.config/nextest.toml (1)
14-16: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicated TOML table declarations.
The repeated
[scripts.setup.*]headers redefine the same tables, so nextest cannot parse this configuration. The extra[[profile.default.overrides]]also creates an empty override.Proposed fix
-[[profile.default.overrides]] [[profile.default.overrides]] filter = 'package(=sdk_test_rust)' test-group = 'rust-sdk-fixture' -[scripts.setup.rust_cargo_unix] [scripts.setup.rust_cargo_unix] -[scripts.setup.rust_cargo_win] [scripts.setup.rust_cargo_win] -[[profile.default.scripts]] [[profile.default.scripts]] -[[profile.default.scripts]] [[profile.default.scripts]] -[scripts.setup.rust_bridge_engine_unix] [scripts.setup.rust_bridge_engine_unix] -[scripts.setup.rust_bridge_engine_win] [scripts.setup.rust_bridge_engine_win] -[[profile.default.scripts]] [[profile.default.scripts]] -[[profile.default.scripts]] [[profile.default.scripts]]Also applies to: 63-77, 86-100
🤖 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/.config/nextest.toml` around lines 14 - 16, Remove the duplicated TOML table declarations in the nextest configuration, including repeated [scripts.setup.*] headers and the extra [[profile.default.overrides]] declaration around the profile overrides. Consolidate each setup table and override entries under a single declaration so nextest parses the configuration without empty or redefined tables.baml_language/crates/baml_release/src/platforms.rs (1)
20-24: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -e printf '\n== platforms.rs outline ==\n' ast-grep outline baml_language/crates/baml_release/src/platforms.rs --view expanded || true printf '\n== search deny_unknown_fields ==\n' rg -n "deny_unknown_fields|experimental|use_cross|smoke|serde_json::from_str|from_reader|Platforms|Platform" baml_language/crates/baml_release/src -g '!target' || true printf '\n== list files in release crate tests ==\n' git ls-files baml_language/crates/baml_release/src baml_language/crates/baml_release/tests 2>/dev/null || trueRepository: BoundaryML/baml
Length of output: 3852
🏁 Script executed:
set -e sed -n '1,220p' baml_language/crates/baml_release/src/platforms.rsRepository: BoundaryML/baml
Length of output: 7968
🏁 Script executed:
set -e printf '\n== cffi test tail ==\n' sed -n '220,280p' baml_language/crates/baml_release/src/platforms.rs printf '\n== look for malformed-key tests ==\n' rg -n "unknown field|deny_unknown_fields|malformed|typo|extra key|serde_json::from_str" baml_language/crates/baml_release/src/platforms.rs baml_language/crates/baml_release/src/manifest.rsRepository: BoundaryML/baml
Length of output: 2210
Reject unknown platform-contract fields.
serdewill silently drop misspelled keys here, so a typo likeexperimentalcan fall back tofalseand change release behavior. Add#[serde(deny_unknown_fields)]to the contract structs, plus a malformed-key test.🤖 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/baml_release/src/platforms.rs` around lines 20 - 24, Add #[serde(deny_unknown_fields)] to the Platforms contract struct and the related platform contract structs used for deserialization, then add a test that supplies an unknown or misspelled key and asserts deserialization fails. Preserve valid platform-contract parsing and existing field defaults.baml_language/crates/baml_tests/tests/env.rs (1)
94-94: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the test file structure first. ast-grep outline baml_language/crates/baml_tests/tests/env.rs --view expanded # Read the relevant sections with line numbers. sed -n '1,240p' baml_language/crates/baml_tests/tests/env.rs | cat -n # Search for any test-serialization helpers, env guards, or global test setup in this test crate. rg -n "serial|serial_test|env_guard|Mutex|OnceLock|set_var|remove_var|OPENAI_API_KEY|ANTHROPIC_API_KEY" baml_language/crates/baml_tests -SRepository: BoundaryML/baml
Length of output: 50371
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the env test file and a nearby test file that also mutates env vars. sed -n '1,220p' baml_language/crates/baml_tests/tests/env.rs | cat -n printf '\n--- google_llm_env.rs ---\n' sed -n '1,260p' baml_language/crates/baml_tests/tests/google_llm_env.rs | cat -n # Check for explicit test serialization / env-guard patterns in this crate. rg -n "serial_test|#[[:space:]]*serial|Mutex|OnceLock|set_var|remove_var|with_var|env guard|restore.*API_KEY" baml_language/crates/baml_tests -S # Check whether the crate/test workspace mentions single-threaded test execution. rg -n "test-threads|RUST_TEST_THREADS|serial_test" baml_language/crates/baml_tests Cargo.toml -SRepository: BoundaryML/baml
Length of output: 26456
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find any repo-level config that forces single-threaded test execution. git ls-files | rg '(^|/)(Cargo\.toml|\.cargo/config(\.toml)?|rustfmt\.toml|justfile|Makefile)$' # Search for test-thread configuration or serial test annotations in the repo. rg -n "test-threads|RUST_TEST_THREADS|serial_test|#[[:space:]]*serial\b|single_thread|threaded" . -S # Inspect the env test comments and any note about isolation in its BAML source. sed -n '1,120p' baml_language/crates/baml_tests/baml_src/ns_env/env.baml | cat -nRepository: BoundaryML/baml
Length of output: 23049
Serialize or guard these env mutations.
baml_language/crates/baml_tests/tests/env.rs#L94,#L124,#L153mutate process-global API key state without restoring it, so these#[tokio::test]cases can race under parallel test execution. Use a per-test env guard or restore the previous value after each mutation.📍 Affects 1 file
baml_language/crates/baml_tests/tests/env.rs#L94-L94(this comment)baml_language/crates/baml_tests/tests/env.rs#L124-L124baml_language/crates/baml_tests/tests/env.rs#L153-L153🤖 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/baml_tests/tests/env.rs` at line 94, Guard or serialize the process-global environment mutations in the tests at baml_language/crates/baml_tests/tests/env.rs lines 94, 124, and 153. Ensure each OPENAI_API_KEY update restores its prior value after the test, or otherwise prevents concurrent execution across all three sites; apply the same protection consistently to each mutation.baml_language/crates/bex_engine/src/conversion.rs (1)
73-86: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== conversion.rs outline ==\n' ast-grep outline baml_language/crates/bex_engine/src/conversion.rs --view expanded || true printf '\n== trace_heap.rs outline ==\n' ast-grep outline baml_language/crates/bex_engine/src/trace_heap.rs --view expanded || true printf '\n== search for cycle detection / HeapPtr / EngineError in relevant crate ==\n' rg -n --hidden --glob '!target' 'HeapPtr|visited|cycle|EngineError|trace_heap' baml_language/crates/bex_engine/src printf '\n== conversion.rs around the commented lines ==\n' sed -n '1,220p' baml_language/crates/bex_engine/src/conversion.rs printf '\n== trace_heap.rs relevant section ==\n' sed -n '1,260p' baml_language/crates/bex_engine/src/trace_heap.rsRepository: BoundaryML/baml
Length of output: 50371
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== convert_heap_ptr_to_external_with_type body ==\n' sed -n '87,330p' baml_language/crates/bex_engine/src/conversion.rs printf '\n== trace_heap cycle-detection implementation ==\n' sed -n '230,360p' baml_language/crates/bex_engine/src/trace_heap.rs printf '\n== trace_heap cyclic-reference test ==\n' sed -n '612,660p' baml_language/crates/bex_engine/src/trace_heap.rs printf '\n== EngineError variants likely relevant to conversion failures ==\n' sed -n '400,520p' baml_language/crates/bex_engine/src/lib.rsRepository: BoundaryML/baml
Length of output: 22211
Fix cycle detection in VM-to-external conversion.
This recursive walk still has no identity-based visited set, so a self-referential array/map/instance can recurse forever or overflow the stack at the host boundary. ThreadHeapPtridentity through the recursive calls, return anEngineErroron back-edges, and add a unit test for a cyclic value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bex_engine/src/conversion.rs` around lines 73 - 86, Update the VM-to-external conversion walk in conversion.rs to track visited HeapPtr identities across recursive array, map, and instance conversions, passing the visited state through each recursive call. Detect back-edges and return an EngineError instead of recursing further, preserving normal conversion for acyclic values. Add a unit test covering a cyclic value and asserting the conversion returns an error.Source: Coding guidelines
baml_language/crates/sys_llm/src/auth_request/mod.rs (1)
119-129: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate environment lookup failures instead of sending an unauthenticated request.
Line 128 converts every
RuntimeIoErrorinto a missing key. A backend failure therefore silently omits authentication and surfaces later as a misleading provider 401. Return aResult<Option<String>, BuildRequestError>and propagate it throughauth_openai/auth_anthropic.Proposed direction
async fn resolve_api_key( client: &PrimitiveClient, provider: LlmProvider, io: &dyn ::sys_types::runtime_io::RuntimeIo, -) -> Option<String> { +) -> Result<Option<String>, BuildRequestError> { if let Some(api_key) = &client.options.api_key { - return Some(api_key.clone()); + return Ok(Some(api_key.clone())); } - let env_var = provider.default_api_key_env_var()?; - io.env_get(env_var.to_string()).await.ok().flatten() + let Some(env_var) = provider.default_api_key_env_var() else { + return Ok(None); + }; + io.env_get(env_var.to_string()) + .await + .map_err(|error| BuildRequestError::Other(error.to_string())) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/sys_llm/src/auth_request/mod.rs` around lines 119 - 129, Change resolve_api_key to return Result<Option<String>, BuildRequestError>, preserving the configured-key and absent-environment-variable cases while propagating RuntimeIoError instead of converting it to None. Update auth_openai and auth_anthropic to propagate this result through their authentication request flows and adjust callers to handle the new error type.baml_language/sdk_tests/crates/rust/llm_functions/customizable/replay_harness.rs (2)
103-132: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Centralize process-environment isolation for the Rust SDK tests.
Default parallel test execution allows one test to overwrite or remove another test's environment.
baml_language/sdk_tests/crates/rust/llm_functions/customizable/replay_harness.rs#L103-L132: hold a shared test lock for the guard's lifetime and restore previous replay values.baml_language/sdk_tests/crates/rust/llm_functions/customizable/test_main.rs#L196-L258: use the same scoped mechanism for provider keys instead of permanentset_varcalls.📍 Affects 2 files
baml_language/sdk_tests/crates/rust/llm_functions/customizable/replay_harness.rs#L103-L132(this comment)baml_language/sdk_tests/crates/rust/llm_functions/customizable/test_main.rs#L196-L258🤖 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/rust/llm_functions/customizable/replay_harness.rs` around lines 103 - 132, Centralize environment isolation across both sites: in baml_language/sdk_tests/crates/rust/llm_functions/customizable/replay_harness.rs:103-132, have RunningServer acquire and retain the shared test lock for its lifetime, snapshot existing BAML_REPLAY_BASE_URL and BAML_REPLAY_API_KEY values, and restore them on Drop instead of removing them unconditionally; in baml_language/sdk_tests/crates/rust/llm_functions/customizable/test_main.rs:196-258, use the same scoped lock-and-restore mechanism for provider environment keys rather than permanent set_var calls.
119-126: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not block indefinitely in
Drop.If the shutdown request fails or the server ignores it,
JoinHandle::join()hangs the entire test process. Pollis_finished()with a deadline and detach/report the thread after timeout.🤖 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/rust/llm_functions/customizable/replay_harness.rs` around lines 119 - 126, The Drop shutdown path around post_shutdown and thread.join must not block indefinitely. Poll JoinHandle::is_finished() until a bounded deadline, joining only when the thread completes; after timeout, detach the handle and report the forced shutdown so the test process can continue.baml_language/sdks/rust/bridge_rust/src/capi.rs (2)
95-104: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant Rust file and search for load/load_inner usage. ast-grep outline baml_language/sdks/rust/bridge_rust/src/capi.rs --view expanded || true printf '\n--- load/load_inner references ---\n' rg -n "fn load|load_inner|block_in_place|spawn_blocking|RuntimeFlavor|try_current|from_process" baml_language/sdks/rust/bridge_rust/src/capi.rs printf '\n--- nearby lines ---\n' sed -n '1,220p' baml_language/sdks/rust/bridge_rust/src/capi.rs printf '\n--- tests in file ---\n' rg -n "#\[test\]|mod tests|tokio::test" baml_language/sdks/rust/bridge_rust/src/capi.rsRepository: BoundaryML/baml
Length of output: 11557
🏁 Script executed:
#!/bin/bash set -euo pipefail ast-grep outline baml_language/sdks/rust/bridge_rust/src/capi.rs --view expanded || true printf '\n--- load/load_inner references ---\n' rg -n "fn load|load_inner|block_in_place|spawn_blocking|RuntimeFlavor|try_current|from_process" baml_language/sdks/rust/bridge_rust/src/capi.rs printf '\n--- nearby lines ---\n' sed -n '1,220p' baml_language/sdks/rust/bridge_rust/src/capi.rs printf '\n--- tests in file ---\n' rg -n "#\[test\]|mod tests|tokio::test" baml_language/sdks/rust/bridge_rust/src/capi.rsRepository: BoundaryML/baml
Length of output: 11557
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "hello"Repository: BoundaryML/baml
Length of output: 159
🌐 Web query:
Tokio block_in_place current_thread runtime unavailable documentation💡 Result:
The tokio::task::block_in_place function is designed to run blocking, synchronous code within an asynchronous context without blocking the executor [1][2]. However, it is explicitly incompatible with the current_thread runtime, and calling it within that context will trigger a panic [1][3]. The reason for this limitation is that block_in_place functions by signaling the executor to hand off other pending tasks to a different worker thread while the current thread is blocked [1][4]. Because the current_thread runtime operates on a single thread and has no other worker threads available to take over these tasks, it cannot safely perform this handoff [1][5]. Consequently, the runtime cannot continue driving other tasks, which would lead to a total deadlock of the system if allowed to proceed [5][6]. If you are using the current_thread runtime and need to execute blocking code, you should consider the following alternatives: 1. Use tokio::task::spawn_blocking: This is generally preferred for offloading blocking tasks to a dedicated thread pool specifically meant for such work, regardless of which runtime you are using [2][4]. 2. Avoid blocking the executor: If possible, refactor the code to be asynchronous [2]. 3. Switch runtimes: If your application requires the specific behavior of block_in_place, you may need to switch to the multi-threaded runtime (rt-multi-thread), which maintains a pool of worker threads capable of handling task handoffs [7][2]. Note that block_in_place is permitted if called outside of any active Tokio runtime, in which case it simply executes the closure normally without any special orchestration [1][3].
Citations:
- 1: https://docs.rs/tokio/latest/tokio/task/fn.block_in_place.html
- 2: https://docs.rs/tokio/latest/tokio/task/
- 3: https://docs.rs/tokio/1/tokio/task/fn.block_in_place.html
- 4: https://github.com/tokio-rs/tokio/blob/306ed1c3/tokio/src/task/blocking.rs
- 5: https://docs.rs/tokio/latest/src/tokio/task/blocking.rs.html
- 6: tokio-rs/tokio#7338
- 7: https://tokio.rs/tokio/topics/bridging
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '--- api() call sites ---\n' rg -n "\bapi\(\)" baml_language/sdks/rust/bridge_rust/src printf '\n--- engine_loaded() call sites ---\n' rg -n "\bengine_loaded\(\)" baml_language/sdks/rust/bridge_rust/src printf '\n--- loader path resolution / download refs ---\n' rg -n "resolve_library_path|download|LoaderEnv|from_process|LoaderError" baml_language/sdks/rust/bridge_rust/src printf '\n--- loader module outline ---\n' fd -a "loader.rs" baml_language/sdks/rust/bridge_rust/src || trueRepository: BoundaryML/baml
Length of output: 244
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '--- api() call sites ---\n' rg -n "\bapi\(\)" baml_language/sdks/rust/bridge_rust/src printf '\n--- engine_loaded() call sites ---\n' rg -n "\bengine_loaded\(\)" baml_language/sdks/rust/bridge_rust/src printf '\n--- loader references ---\n' rg -n "resolve_library_path|from_process|LoaderEnv|download|cargo|fs::|std::thread::sleep" baml_language/sdks/rust/bridge_rust/src printf '\n--- candidate loader files ---\n' git ls-files baml_language/sdks/rust/bridge_rust/src | sed -n '1,120p'Repository: BoundaryML/baml
Length of output: 244
🏁 Script executed:
#!/bin/bash set -euo pipefail printf -- '--- api() call sites ---\n' rg -n "\bapi\(\)" baml_language/sdks/rust/bridge_rust/src printf -- '\n--- engine_loaded() call sites ---\n' rg -n "\bengine_loaded\(\)" baml_language/sdks/rust/bridge_rust/src printf -- '\n--- loader/path resolution refs ---\n' rg -n "resolve_library_path|from_process|LoaderEnv|download|library path|OnceLock|get_or_init" baml_language/sdks/rust/bridge_rust/src printf -- '\n--- files in src ---\n' git ls-files baml_language/sdks/rust/bridge_rust/srcRepository: BoundaryML/baml
Length of output: 15617
🏁 Script executed:
#!/bin/bash set -euo pipefail printf -- '--- runtime.rs relevant sections ---\n' sed -n '1,180p' baml_language/sdks/rust/bridge_rust/src/runtime.rs printf -- '\n--- completion.rs relevant sections ---\n' sed -n '160,260p' baml_language/sdks/rust/bridge_rust/src/completion.rs printf -- '\n--- loader/mod.rs relevant sections ---\n' sed -n '90,140p' baml_language/sdks/rust/bridge_rust/src/loader/mod.rsRepository: BoundaryML/baml
Length of output: 11220
Avoid lazy engine loading on current-thread Tokio runtimes.
api()is reachable from async call paths, andload()still resolves/downloads the library synchronously onRuntimeFlavor::CurrentThread, blocking the only executor thread. Move initialization tospawn_blockingor require a preload step before async dispatch.🤖 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/rust/bridge_rust/src/capi.rs` around lines 95 - 104, Update load so initialization never performs synchronous library resolution or downloading on a current-thread Tokio runtime. Preserve the existing block_in_place path for multi-thread runtimes, and route current-thread async callers through spawn_blocking or enforce an explicit preload before api() dispatch, while retaining synchronous loading outside Tokio.
159-165: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Validate the ABI prefix before constructing
&BamlApiV1.
&*get_api_v1()already requires a non-null allocation containing a fully validBamlApiV1. A null or shorter legacy table therefore causes undefined behavior beforestruct_sizeis checked. Rust references must be non-null and cover a valid value. (doc.rust-lang.org)Read a minimal
{ abi_version, struct_size }prefix first, validate it, and only then cast to the complete table.🤖 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/rust/bridge_rust/src/capi.rs` around lines 159 - 165, Update the ABI loading logic around get_api_v1 so it reads a raw, minimal prefix containing abi_version and struct_size before constructing any reference. Validate the returned pointer is non-null and the prefix is large enough for the required fields, then reject incompatible versions or undersized tables; only after these checks cast the pointer to &BamlApiV1 and retain the existing full-table validation behavior.baml_language/sdks/rust/bridge_rust/src/completion.rs (1)
182-190: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Complete the receiver when the callback panics.
This path only logs the panic. The associated receiver can remain pending forever because
Statehas no failure variant and neither wait path has a timeout. Record a failed state and wake/notify the receiver so runtime calls return an internal error instead of hanging.🤖 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/rust/bridge_rust/src/completion.rs` around lines 182 - 190, Update the panic handling around the completion callback to record a failed state in State, then wake or notify the associated receiver so both wait paths return an internal error instead of remaining pending. Preserve the existing stderr logging and C-boundary panic containment, and use the receiver’s established completion/notification mechanism.baml_language/sdks/rust/bridge_rust/src/loader/download.rs (2)
36-48: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not install executable artifacts when checksum retrieval fails.
A missing, malformed, or unreachable sidecar disables verification and still installs bytes that are subsequently loaded as native code. This makes the integrity check bypassable by suppressing the sidecar.
Require a trusted/pinned checksum, or fail closed when verification cannot be performed.
Also applies to: 105-119
🤖 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/rust/bridge_rust/src/loader/download.rs` around lines 36 - 48, Update the checksum handling around fetch_checksum and the subsequent download/install flow to fail closed when the checksum is missing, malformed, or unreachable. Require a trusted or pinned checksum before permitting the artifact to be downloaded and installed; remove the path that returns None and proceeds without verification, including the corresponding logic around the additionally referenced download flow.
134-146: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve atomic installation when rename fails.
copy_filewrites directly todest_path; a crash or concurrent resolver can observe a truncated shared library, whileresolve_library_pathtreats any existing file as a cache hit. Avoid the non-atomic fallback, or add locking and publish only through an atomic final operation.🤖 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/rust/bridge_rust/src/loader/download.rs` around lines 134 - 146, Update the rename-failure path in the download flow around copy_file so installation remains atomic. Do not copy directly to dest_path; instead write the fallback to a temporary destination and publish it with an atomic rename, or otherwise use locking that prevents readers from observing partial files. Preserve the existing LoaderError::DownloadFailed reporting and success logging.baml_language/sdks/rust/bridge_rust/src/loader/mod.rs (1)
99-117: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Synchronize path configuration with the start of engine loading.
A load can snapshot
EXPLICIT_PATH, release the mutex, and continue initializing whileengine_loaded()still returns false. A concurrent setter then stores a new path and returns successfully, but the in-progress load ignores it.Track a
load_startedstate under the same lock—or freeze configuration before taking the snapshot—and add a concurrency regression test.🤖 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/rust/bridge_rust/src/loader/mod.rs` around lines 99 - 117, Synchronize set_shared_library_path with the beginning of engine loading: update the shared state guarded by EXPLICIT_PATH so loading marks configuration as started before snapshotting the path, and have set_shared_library_path reject and warn when that state is active even if engine_loaded() is still false. Update the relevant loader initialization flow and add a concurrency regression test covering a setter racing with the initial load.baml_language/sdks/rust/sdkgen_rust/src/analyze.rs (1)
447-502: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ensure renamed modules are actually unique.
Appending one
_can collide with an existing sibling namespace or type. For example, a typeFooplus namespacesFooandFoo_maps both namespaces toFoo_. Because crate assembly keys leaves byanalysis.renamed(...), this can merge distinct namespaces or generate duplicate Rust items.Keep appending
_until the name is unused by both sibling modules and emitted types, and add a unit test for this case.🤖 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/rust/sdkgen_rust/src/analyze.rs` around lines 447 - 502, Update compute_renames to choose each colliding child segment by repeatedly appending "_" until the resulting name is unused among both sibling namespace paths in all_paths and emitted type names in types_in for the parent; preserve ancestor renames and apply the selected name to descendants. Add a unit test covering a type Foo with namespaces Foo and Foo_, asserting the renamed namespaces remain distinct.baml_language/sdks/rust/sdkgen_rust/src/emit/function.rs (1)
87-104: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Reserve every generator-owned identifier before emission.
Generated wrapper and runtime names currently bypass namespace collision analysis, allowing valid BAML symbols to produce uncompilable Rust.
baml_language/sdks/rust/sdkgen_rust/src/emit/function.rs#L87-L104: reserve/de-conflict sync and_asyncwrapper names across each leaf.baml_language/sdks/rust/sdkgen_rust/src/lib.rs#L351-L370: reserve_inlinedbaml,_runtime, and the rootinitexport.Add regression cases for
fooplusfoo_async, and for root symbols using each internal name. Would you like me to open an issue for this work?📍 Affects 2 files
baml_language/sdks/rust/sdkgen_rust/src/emit/function.rs#L87-L104(this comment)baml_language/sdks/rust/sdkgen_rust/src/lib.rs#L351-L370🤖 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/rust/sdkgen_rust/src/emit/function.rs` around lines 87 - 104, Reserve and de-conflict generator-owned identifiers before emitting code: update the function emission logic around sync_name and async_name so each leaf handles collisions between foo and foo_async, and update the root generation logic in baml_language/sdks/rust/sdkgen_rust/src/lib.rs at lines 351-370 to reserve _inlinedbaml, _runtime, and the root init export. Add regression coverage for both function-name collisions and root symbols using each reserved internal name.baml_language/sdks/rust/sdkgen_rust/src/unions.rs (1)
173-214: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reject union arms that overlap after wire or Rust lowering.
int[] | string[]accepts an empty list as the first arm tried, silently guessing its variant. A transparentAliasToString | stringcan also emit conflictingFrom<String>implementations. Compare arm wire/translated shapes and fail closed on overlap; add regression tests for containers and aliases.Also applies to: 252-290
🤖 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/rust/sdkgen_rust/src/unions.rs` around lines 173 - 214, Extend shape_error to detect overlapping union arms after wire encoding or Rust lowering, not only duplicate variant names and string-literal conflicts. Ensure container arms such as int[] | string[] with shared empty-list representations and transparent aliases such as AliasToString | string are rejected before enum generation, preventing ambiguous decoding and conflicting From<String> implementations. Keep analyze and collect using this shared validation, and add regression tests covering both container overlap and alias/string overlap.scripts/baml-language-version (1)
354-363: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject non-string registry versions instead of coercing them.
null, booleans, numbers, and structured values currently become strings such as"None"or"True"and can reach package stamping. Validate each required entry as a non-empty JSON string before constructingReleasePlan.Proposed fix
- registry_versions = {str(key): str(value) for key, value in raw_registry_versions.items()} - missing = [name for name in PLAN_REGISTRIES if name not in registry_versions] + missing = [name for name in PLAN_REGISTRIES if name not in raw_registry_versions] if missing: fail(f"{path} registry_versions is missing {', '.join(missing)}") + registry_versions = {} + for name in PLAN_REGISTRIES: + value = raw_registry_versions[name] + if not isinstance(value, str) or not value: + fail(f"{path} registry_versions[{name!r}] must be a non-empty string") + registry_versions[name] = value📝 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.try: raw_registry_versions = data["registry_versions"] except KeyError as exc: fail(f"{path} is missing {exc.args[0]!r}") if not isinstance(raw_registry_versions, dict): fail(f"{path} registry_versions must be a JSON object") missing = [name for name in PLAN_REGISTRIES if name not in raw_registry_versions] if missing: fail(f"{path} registry_versions is missing {', '.join(missing)}") registry_versions = {} for name in PLAN_REGISTRIES: value = raw_registry_versions[name] if not isinstance(value, str) or not value: fail(f"{path} registry_versions[{name!r}] must be a non-empty string") registry_versions[name] = value🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/baml-language-version` around lines 354 - 363, Update the registry_versions validation in the data-loading flow to require every required entry from PLAN_REGISTRIES to be a non-empty JSON string, rejecting nulls, booleans, numbers, and structured values. Remove the str() coercion when constructing registry_versions, and perform validation before creating ReleasePlan while preserving the existing missing-registry checks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The HIR/PPIR
ItemTreeis the per-file layer that is regenerated every time that file changes. This means that directly relying on theItemTreewill cause queries to be recomputed every time a file changes even if the query itself is unrelated to the changed part of the file. This PR adds "firewall" queries that are more granular queries which allow the dependency scope to be constrained to a single item using salsa backdating. It also removes span information from much of the queries and moves it to parallel source map queries so span changes (e.g. changing whitespace) will not force a full recomputation of the TIR. This includes adding the HIR/PPIRTypeRefwhich is a span-less version of the AST'sTypeExpr.There is still work to be done after this PR: not all consumers have been switched over to use the firewall queries, meaning they will unnecessarily recalculate based on the coarse dependency and/or span changes.
Summary by CodeRabbit
Performance
Bug Fixes
Selfreferences.Tests