baml_language: standalone execution (baml run)#3375
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
e20c4ec to
9a66447
Compare
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant User as User CLI
participant CLI as baml CLI
participant Project as Project Loader
participant Engine as BexEngine
participant VM as BexVm
participant Resolver as Function Resolver
participant Builtins as sys.argv()
User->>CLI: invoke `baml run ...`
CLI->>Project: load/compile project or read single .baml
Project-->>CLI: files / snapshot
CLI->>Engine: BexEngine::new(bytecode, sys_ops, events, argv)
Engine->>VM: BexVm::new(..., argv)
VM->>Builtins: provide argv via sys.argv()
CLI->>Resolver: resolve target (--function / main / scripts / namespace)
Resolver-->>CLI: resolved function
CLI->>Engine: call_function(target, args)
Engine->>VM: create VM, execute call
VM-->>Engine: return result
Engine-->>CLI: result
CLI->>User: print formatted output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)✅ Unit Test PR creation complete.
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 |
Binary size checks passed✅ 7 passed
Generated by |
Merging this PR will not alter performance
|
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
baml_language/crates/bex_engine/src/lib.rs (1)
521-575:⚠️ Potential issue | 🔴 CriticalInitialize
baml.argvbefore$initruns.
$initevaluates top-level let initializers, but this constructor doesn't populate thebaml.argvslot until after every package init function has already completed. Any initializer that readsbaml.argvwill therefore see the default global value instead of the CLI arguments, which breaks the new standalone-execution flow.Move the argv allocation/store to immediately after
globalsis created and before the$initloop.Suggested reorder
let mut globals = GlobalPool::from_vec(globals_vec); + if let Some(slot) = argv_global_index { + let mut tlab = Tlab::new(Arc::clone(&heap)); + let values: Vec<Value> = argv + .into_iter() + .map(|s| Value::Object(tlab.alloc_string(s))) + .collect(); + let array_ptr = tlab.alloc_array(values); + globals[bex_vm_types::GlobalIndex::from_raw(slot)] = Value::Object(array_ptr); + } + // Run $init for each package in dependency order. for init_name in &package_init_order { if let Some((init_ptr, _kind)) = resolved_function_names.get(init_name.as_str()) { let mut vm = BexVm::new( Arc::clone(&heap), @@ - // Populate the `baml.argv` global slot with a heap-allocated string[]. - if let Some(slot) = argv_global_index { - let mut tlab = Tlab::new(Arc::clone(&heap)); - let values: Vec<Value> = argv - .into_iter() - .map(|s| Value::Object(tlab.alloc_string(s))) - .collect(); - let array_ptr = tlab.alloc_array(values); - globals[bex_vm_types::GlobalIndex::from_raw(slot)] = Value::Object(array_ptr); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_engine/src/lib.rs` around lines 521 - 575, The argv (baml.argv) population must happen before running package init functions: after creating the initial globals but before the for init loop over package_init_order, allocate a Tlab (using Arc::clone(&heap)), build the string Value::Object entries, alloc_array them and store into globals at bex_vm_types::GlobalIndex::from_raw(slot) (using argv_global_index). Move the current argv allocation/store (the Tlab creation, Value::Object(tlab.alloc_string(...)) mapping, tlab.alloc_array call and globals[...] = ...) to immediately after globals is initialized and before any BexVm::new / $init execution so initializers reading baml.argv see the CLI args.baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
2316-2378:⚠️ Potential issue | 🟠 MajorPreserve sys-op dispatch for callable
FreeLets.
lower_call()now treatsMemberResolution::FreeLetas callable, butcheck_sys_op()still only extracts builtin IO metadata from direct function resolutions. That means a top-level alias likelet sleep = baml.time.sleep; sleep(...)will fall through the normalCallpath instead ofdispatch_future/await_, so the alias no longer behaves like the underlying sys-op.Please carry enough metadata to recover the wrapped callable here, or teach
check_sys_op()how to inspectDefinition::Let/FreeLetinitializers before deciding.Based on learnings, "New language features for BAML require coordinated updates across Parser (parser-database), IR/validation (baml-core), Compiler (baml-compiler), and VM (baml-vm)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_compiler2_mir/src/lower.rs` around lines 2316 - 2378, check_sys_op currently ignores MemberResolution::FreeLet so aliases like `let sleep = baml.time.sleep` lose builtin IO dispatch; update check_sys_op to treat FreeLet as potentially callable by retrieving the let definition/initializer (the Definition::Let/FreeLet or the let location stored in MemberResolution::FreeLet), inspect its initializer expression to recover the underlying function location (the same way you handle single-segment Path resolution or the MemberResolution::FreeFunction/Method func_loc), then call baml_compiler2_ppir::function_body(...) and check for FunctionBody::Builtin(BuiltinKind::Io) as you do for functions; alternatively ensure the metadata propagated by lower_call preserves the wrapped func_loc and read that here.
🧹 Nitpick comments (1)
baml_language/crates/bex_engine/tests/common/mod.rs (1)
76-82: Consider threadingargvthrough the shared test harness.Hardcoding
vec![]here means any engine test that wants to coverbaml.argvnow has to bypassEngineProgram/ these helpers and hand-roll engine setup. Adding an optional argv field now would keep the common path useful for the new feature too.Also applies to: 210-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_engine/tests/common/mod.rs` around lines 76 - 82, The shared test harness currently hardcodes argv as vec![] when calling BexEngine::new (in tests/common/mod.rs), preventing tests from exercising baml.argv; add an optional argv field to the common harness/EngineProgram struct and thread it through to the BexEngine::new calls (replace the hardcoded vec![] with the new field, defaulting to empty when not provided), and update the other occurrence around lines 210-216 similarly so all helper paths pass argv through to BexEngine::new.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@baml_language/crates/baml_cli/src/lib.rs`:
- Line 18: Update the crate-level documentation to reflect the new CLI wiring:
remove or revise the top-of-file TODO and the doc comment on the run_cli
function so they no longer state the crate supports only LSP; mention that this
crate now also exposes a CLI command via the run_command module (e.g., "baml
run") and briefly document run_cli's role in initializing either LSP or CLI
execution paths and reference run_command where appropriate.
In `@baml_language/crates/baml_cli/src/run_command.rs`:
- Around line 199-203: The help trigger currently matches any occurrence of
"--help" in effective_target_args which also captures it as a parameter value;
change the condition to only treat the dedicated invocation shape (exactly the
standalone extra arg) as target help. Replace the any(|a| a == "--help") check
with a strict check such as ensuring effective_target_args has length 1 and its
sole element equals "--help" before calling
self.print_target_help(&function_name, ¶m_names, ¶m_types, &engine) and
returning Success.
- Around line 431-472: The code currently uses
std::fs::canonicalize(&self.from).ok() which swallows canonicalization errors
and treats a bad --from as "no explicit project"; change this so that when the
user supplied a --from (self.from is non-empty / provided) any Err from
canonicalize is surfaced as a hard error (return Err or exit with a clear
message) instead of falling back to the temp dir. Concretely, replace the .ok()
call with a match on canonicalize(&self.from): if Ok(path) set from=Some(path);
if Err(e) and self.from was provided, propagate the error (or print and return
Err) with context mentioning self.from; only treat missing/unset --from as "no
explicit project". Keep references to the existing variables/functions:
self.from, from, has_explicit_project, ProjectDatabase::new, and project_root.
- Around line 46-72: The parser parse_script_body currently uses
body.split_whitespace() which breaks quoted/escaped arguments; replace that
tokenization with a shell-style tokenizer (e.g., shell_words::split or similar)
to preserve quotes and escapes, so tokens like "--name \"Ada Lovelace\"" or
inline JSON remain single tokens. Update the token vector type to Vec<String>,
call the shell tokenizer and propagate any error into the Result, then adapt the
loop in parse_script_body to iterate over the Vec<String> (use
tokens[i].as_str() or similar) while preserving the existing handling of "--",
"--function", and extra_args; also add the crate import/use and error mapping
for the tokenizer failure.
- Around line 1521-1531: Tests like test_load_json_source_file create files in a
fixed temp path which makes them flaky when run in parallel; update
test_load_json_source_file (and the other similar test mentioned) to use unique
temporary directories (e.g., tempfile::TempDir or append a UUID/timestamp +
thread id to the dir name) instead of a constant "baml_test_json" under
temp_dir(), write the test file into that TempDir, call
load_json_source(&format!("@{}", path.display())) as before, and rely on
TempDir's automatic cleanup (or explicitly remove the unique dir) to avoid
collisions between parallel cargo test runs.
- Around line 684-691: The JSON-to-external conversion is currently untyped:
callers that build the argument HashMap after load_json_source() call use
json_to_external(value) without the target Ty, which causes enums/objects/arrays
to be marshaled as plain strings/empty Instances/lists of strings; fix by
changing json_to_external to accept a target Ty (or add
json_to_external_with_ty) and update the call sites that construct the args map
(the loop after load_json_source()) and the complex `--name <json>` path to pass
the correct parameter Ty (derive the Ty from the parameter definition for
map/enum/class/list types so enums become Variant, objects become Instance with
class_name set, and arrays use the proper element_type) and likewise update any
other call sites that convert JSON to External to use the typed conversion.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 4159-4174: The bailout currently only checks the immediate prefix
of item_path; instead iterate over all non-empty proper prefixes of item_path
(for each split point i from 1 to item_path.len()-1) and call
pkg_items.lookup_value on that prefix (use prefix_ns = &item_path[..i-1] and
prefix_name = &item_path[i-1] or equivalently treat the prefix's last segment as
prefix_name); if any lookup_value returns Some, return None so the chain is
treated as a method/member access rather than an unresolved package path. Ensure
you use the same identifiers from the diff (item_path, prefix_name, prefix_ns,
pkg_items.lookup_value) when implementing the loop.
- Around line 3241-3267: The code returns Ty::Unknown when a package-visible let
has no declared_type (inside the lookup handling for Definition::Let(let_loc)),
which drops the initializer-inferred type; instead, when let_data.declared_type
is None fetch the let initializer from let_data (e.g. let_data.initializer), run
it through the compiler's expression type inference (use the existing inference
API/module used elsewhere in this crate, e.g. the crate::inference entry used
for other expressions) to produce a Ty, collect any diagnostics similarly to the
declared-type path, and return that inferred Ty (while still inserting the same
MemberResolution::FreeLet for expr_id); only fall back to Ty::Unknown if no
initializer exists or inference fails.
In `@baml_language/crates/baml_compiler2_tir/src/inference.rs`:
- Around line 52-54: Update the doc comment on the FreeFunction variant in
inference.rs to use the correct fully-qualified example for builtin stdlib
functions: change the example from `env.get` to `baml.env.get` and adjust the
descriptive breakup to reflect package="baml", namespace=["env"], name="get" so
the comment accurately documents how free-function member resolution is
accessed.
In `@baml_language/crates/baml_lsp2_actions/src/definition.rs`:
- Around line 379-385: The go-to-definition misses top-level lets because
resolve_member_at()/resolve_field_access_at() bails when no enclosing Function
is found; update resolve_member_at (and its caller resolve_field_access_at) to
handle MemberResolution::FreeLet even when there is no Function ancestor by
falling through to the existing MemberResolution::FreeLet handling path instead
of returning None, and ensure you use the same logic that converts
MemberResolution::FreeLet { let_loc } into a Definition::Let and then call
utils::definition_span(db, def) to build the Location; keep the existing
Location construction for Let(*let_loc) so top-level bindings like `let args =
baml.argv` resolve.
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 1155-1190: Create a single predicate for "user-callable" and reuse
it from both function_exists() and user_functions(): add a helper like
is_user_callable(name: &str) -> bool (or is_user_callable_resolved(name: &str,
entry: &(ptr, kind)) -> bool) that enforces the rule "starts with 'user.' and
does not contain any '$' (including $init/$lambda) and is Bytecode
FunctionKind"; update function_exists() to call
resolve_function_name(name).filter(|entry| is_user_callable_resolved(name,
entry)).is_some() (or resolve then call is_user_callable), and refactor
user_functions() to call the same predicate instead of duplicating the name
checks and kind match on resolved_function_names; reference functions:
function_exists, user_functions, resolve_function_name, resolved_function_names,
and bex_vm_types::FunctionKind::Bytecode.
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 2316-2378: check_sys_op currently ignores
MemberResolution::FreeLet so aliases like `let sleep = baml.time.sleep` lose
builtin IO dispatch; update check_sys_op to treat FreeLet as potentially
callable by retrieving the let definition/initializer (the
Definition::Let/FreeLet or the let location stored in
MemberResolution::FreeLet), inspect its initializer expression to recover the
underlying function location (the same way you handle single-segment Path
resolution or the MemberResolution::FreeFunction/Method func_loc), then call
baml_compiler2_ppir::function_body(...) and check for
FunctionBody::Builtin(BuiltinKind::Io) as you do for functions; alternatively
ensure the metadata propagated by lower_call preserves the wrapped func_loc and
read that here.
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 521-575: The argv (baml.argv) population must happen before
running package init functions: after creating the initial globals but before
the for init loop over package_init_order, allocate a Tlab (using
Arc::clone(&heap)), build the string Value::Object entries, alloc_array them and
store into globals at bex_vm_types::GlobalIndex::from_raw(slot) (using
argv_global_index). Move the current argv allocation/store (the Tlab creation,
Value::Object(tlab.alloc_string(...)) mapping, tlab.alloc_array call and
globals[...] = ...) to immediately after globals is initialized and before any
BexVm::new / $init execution so initializers reading baml.argv see the CLI args.
---
Nitpick comments:
In `@baml_language/crates/bex_engine/tests/common/mod.rs`:
- Around line 76-82: The shared test harness currently hardcodes argv as vec![]
when calling BexEngine::new (in tests/common/mod.rs), preventing tests from
exercising baml.argv; add an optional argv field to the common
harness/EngineProgram struct and thread it through to the BexEngine::new calls
(replace the hardcoded vec![] with the new field, defaulting to empty when not
provided), and update the other occurrence around lines 210-216 similarly so all
helper paths pass argv through to BexEngine::new.
🪄 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: fa00a989-6a55-4211-908c-abc6c0fe486d
⛔ Files ignored due to path filters (3)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_tests/snapshots/__baml_std__/baml_tests____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snap
📒 Files selected for processing (26)
baml_language/crates/baml_cli/Cargo.tomlbaml_language/crates/baml_cli/src/commands.rsbaml_language/crates/baml_cli/src/lib.rsbaml_language/crates/baml_cli/src/run_command.rsbaml_language/crates/baml_cli/src/test_command.rsbaml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/item_tree.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_lsp2_actions/src/definition.rsbaml_language/crates/baml_tests/src/engine.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/build_request.rsbaml_language/crates/bex_engine/tests/cancellation.rsbaml_language/crates/bex_engine/tests/collect_tests.rsbaml_language/crates/bex_engine/tests/common/mod.rsbaml_language/crates/bex_engine/tests/concurrent.rsbaml_language/crates/bex_engine/tests/gc.rsbaml_language/crates/bex_engine/tests/llm_render.rsbaml_language/crates/bex_engine/tests/orchestration.rsbaml_language/crates/bex_engine/tests/registry.rsbaml_language/crates/bex_engine/tests/tracing.rsbaml_language/crates/bex_project/src/project.rs
There was a problem hiding this comment.
♻️ Duplicate comments (3)
baml_language/crates/baml_compiler2_tir/src/builder.rs (2)
3237-3251:⚠️ Potential issue | 🟠 MajorHandle
Definition::Lethere as a value.
lookup_valuecan now return top-level lets, but this helper still only acceptsDefinition::Function. That meanspkg.ns.some_letstill falls through toUnknown, andpkg.ns.some_let.membernever gets a typed base for normal member resolution. Please add aDefinition::Letbranch here and record a dedicated resolution for downstream MIR/LSP.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_compiler2_tir/src/builder.rs` around lines 3237 - 3251, lookup_value can return top-level lets but the current code only handles Definition::Function; add a branch for Definition::Let(let_loc) (matching the same place that currently handles Definition::Function in builder.rs) and insert a dedicated resolution into self.resolutions for that expr_id (analogous to the existing crate::inference::MemberResolution::FreeFunction insertion) so downstream MIR/LSP get a typed base (e.g., insert MemberResolution::TopLevelLet { let_loc } or the project’s equivalent variant). Ensure you reference lookup_val, expr_id and use the same lookup context (pkg_items.lookup_value & path) when adding the new branch.
4130-4145:⚠️ Potential issue | 🟠 MajorScan every earlier prefix before reporting an unresolved package path.
This bailout only checks the immediate prefix. Chains like
baml.argv.map.filterstill misfire:lookup_value(["argv"], "map")is false even thoughlookup_value([], "argv")would show the value boundary and the remaining segments should be resolved as members on that value.♻️ Proposed fix
- if !item_path.is_empty() { - let prefix_name = &item_path[item_path.len() - 1]; - let prefix_ns = &item_path[..item_path.len() - 1]; - let prefix_is_value = pkg_items.lookup_value(prefix_ns, prefix_name).is_some(); - if prefix_is_value { - return None; - } - } + for split in (1..=item_path.len()).rev() { + let prefix_name = &item_path[split - 1]; + let prefix_ns = &item_path[..split - 1]; + if pkg_items.lookup_value(prefix_ns, prefix_name).is_some() { + return None; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_compiler2_tir/src/builder.rs` around lines 4130 - 4145, The current bailout in builder.rs checks only the immediate prefix of item_path before returning None, which misses earlier prefixes (e.g., `baml.argv.map.filter`); change the logic in the code that currently uses item_path and pkg_items.lookup_value so it scans every possible earlier prefix: for each split point in item_path (taking prefix_ns = &item_path[..i] and prefix_name = &item_path[i]), call pkg_items.lookup_value(prefix_ns, prefix_name) and if any returns Some(...) treat it as a value boundary and return None; only if no prefix resolves as a value should the unresolved package path be reported. Ensure you reference the variables item_path and pkg_items.lookup_value in the updated loop.baml_language/crates/bex_engine/src/lib.rs (1)
1151-1186:⚠️ Potential issue | 🟠 MajorInconsistent "user-callable" predicate between
function_existsanduser_functions.
function_existsaccepts any name resolvable viaresolve_function_name(including class methods,$init/$init_test,$lambda_*, and synthetic helpers), whileuser_functionsapplies a substring-based filter onuser./$init/.$/$lambda. This leavesbaml runbehavior inconsistent: a name rejected from--listcan still pass the CLI's--functionexistence check. The filter also does not catch synthetic helpers embedding$without a leading.(e.g.Foo$render_prompt).Consider centralizing a single
is_user_callable(name, kind)predicate and reusing it in both methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_engine/src/lib.rs` around lines 1151 - 1186, Create a single predicate (e.g. is_user_callable(name: &str, kind: &bex_vm_types::FunctionKind) -> bool) that encodes the same rules used by --list (exclude non-Bytecode via FunctionKind::Bytecode, exclude constructors/init/$lambda and any synthetic helpers containing '$' regardless of position, and require the "user." prefix semantics you want) and then use that predicate from both user_functions and function_exists: in user_functions apply it when iterating resolved_function_names (instead of the current substring filters) and in function_exists call resolve_function_name(name) and pass the resolved kind into is_user_callable to decide existence; keep returning UserFunctionInfo as before for matching Object::Function entries.
🧹 Nitpick comments (2)
baml_language/crates/bex_engine/src/lib.rs (2)
1156-1186: Add a unit test foruser_functionsfiltering.New public API with non-trivial filter logic (user prefix, kind, synthetic-name exclusion, param/return extraction) and no direct test coverage. A small unit test over a hand-built
resolved_function_namesmap would guard against regressions as the predicate evolves.As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_engine/src/lib.rs` around lines 1156 - 1186, Add a unit test for the public method user_functions that builds a BexEngine (or constructs the same struct) and populates resolved_function_names with multiple entries: a valid "user.foo" entry whose (ptr -> Object::Function) has param_names, param_types and return_type set; entries that should be filtered out such as non-"user." names, entries with "$init", ".$" or "$lambda" in the name, and entries whose FunctionKind is not bex_vm_types::FunctionKind::Bytecode. Call user_functions() and assert the result contains only the expected UserFunctionInfo for the valid "user.*" bytecode entry with matching qualified_name, display_name (strip_prefix("user.")), param_names/param_types and return_type, and that all excluded entries are absent.
1160-1165: Nit: overlapping substring filters.
name.contains("$init")already subsumes$init_testand related variants, and combined withname.contains(".$")the checks overlap for names likeuser.$init/user.$lambda_*. A single check on the trailing segment (after the last.) starting with$would be clearer and more robust than fourcontainspredicates — and would also cover names likeuser.pkg.$anythingwithout relying onFunctionKind::Bytecodeas the backstop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_engine/src/lib.rs` around lines 1160 - 1165, The filter currently uses overlapping substring checks on the tuple key (the closure argument name) to exclude generated symbols; instead extract the trailing segment after the last '.' from the closure argument (name) inside the same .filter(...) and reject any entry whose trailing segment starts with '$' (i.e., segment.starts_with('$')), while still requiring name.starts_with("user."); update the closure used in the .filter call in lib.rs to perform this single, clearer check rather than the multiple name.contains(...) predicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 3237-3251: lookup_value can return top-level lets but the current
code only handles Definition::Function; add a branch for
Definition::Let(let_loc) (matching the same place that currently handles
Definition::Function in builder.rs) and insert a dedicated resolution into
self.resolutions for that expr_id (analogous to the existing
crate::inference::MemberResolution::FreeFunction insertion) so downstream
MIR/LSP get a typed base (e.g., insert MemberResolution::TopLevelLet { let_loc }
or the project’s equivalent variant). Ensure you reference lookup_val, expr_id
and use the same lookup context (pkg_items.lookup_value & path) when adding the
new branch.
- Around line 4130-4145: The current bailout in builder.rs checks only the
immediate prefix of item_path before returning None, which misses earlier
prefixes (e.g., `baml.argv.map.filter`); change the logic in the code that
currently uses item_path and pkg_items.lookup_value so it scans every possible
earlier prefix: for each split point in item_path (taking prefix_ns =
&item_path[..i] and prefix_name = &item_path[i]), call
pkg_items.lookup_value(prefix_ns, prefix_name) and if any returns Some(...)
treat it as a value boundary and return None; only if no prefix resolves as a
value should the unresolved package path be reported. Ensure you reference the
variables item_path and pkg_items.lookup_value in the updated loop.
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 1151-1186: Create a single predicate (e.g. is_user_callable(name:
&str, kind: &bex_vm_types::FunctionKind) -> bool) that encodes the same rules
used by --list (exclude non-Bytecode via FunctionKind::Bytecode, exclude
constructors/init/$lambda and any synthetic helpers containing '$' regardless of
position, and require the "user." prefix semantics you want) and then use that
predicate from both user_functions and function_exists: in user_functions apply
it when iterating resolved_function_names (instead of the current substring
filters) and in function_exists call resolve_function_name(name) and pass the
resolved kind into is_user_callable to decide existence; keep returning
UserFunctionInfo as before for matching Object::Function entries.
---
Nitpick comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 1156-1186: Add a unit test for the public method user_functions
that builds a BexEngine (or constructs the same struct) and populates
resolved_function_names with multiple entries: a valid "user.foo" entry whose
(ptr -> Object::Function) has param_names, param_types and return_type set;
entries that should be filtered out such as non-"user." names, entries with
"$init", ".$" or "$lambda" in the name, and entries whose FunctionKind is not
bex_vm_types::FunctionKind::Bytecode. Call user_functions() and assert the
result contains only the expected UserFunctionInfo for the valid "user.*"
bytecode entry with matching qualified_name, display_name
(strip_prefix("user.")), param_names/param_types and return_type, and that all
excluded entries are absent.
- Around line 1160-1165: The filter currently uses overlapping substring checks
on the tuple key (the closure argument name) to exclude generated symbols;
instead extract the trailing segment after the last '.' from the closure
argument (name) inside the same .filter(...) and reject any entry whose trailing
segment starts with '$' (i.e., segment.starts_with('$')), while still requiring
name.starts_with("user."); update the closure used in the .filter call in lib.rs
to perform this single, clearer check rather than the multiple
name.contains(...) predicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6b7db243-c791-47d7-aac7-38430bb82fa1
⛔ Files ignored due to path filters (3)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_tests/snapshots/__baml_std__/baml_tests____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snap
📒 Files selected for processing (26)
baml_language/crates/baml_builtins2/baml_std/baml/ns_sys/sys.bamlbaml_language/crates/baml_cli/Cargo.tomlbaml_language/crates/baml_cli/src/lib.rsbaml_language/crates/baml_cli/src/run_command.rsbaml_language/crates/baml_cli/src/test_command.rsbaml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_lsp2_actions/src/definition.rsbaml_language/crates/baml_tests/src/engine.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/build_request.rsbaml_language/crates/bex_engine/tests/cancellation.rsbaml_language/crates/bex_engine/tests/collect_tests.rsbaml_language/crates/bex_engine/tests/common/mod.rsbaml_language/crates/bex_engine/tests/concurrent.rsbaml_language/crates/bex_engine/tests/gc.rsbaml_language/crates/bex_engine/tests/llm_render.rsbaml_language/crates/bex_engine/tests/orchestration.rsbaml_language/crates/bex_engine/tests/registry.rsbaml_language/crates/bex_engine/tests/tracing.rsbaml_language/crates/bex_project/src/project.rsbaml_language/crates/bex_vm/src/package_baml/sys.rsbaml_language/crates/bex_vm/src/vm.rs
✅ Files skipped from review due to trivial changes (5)
- baml_language/crates/bex_engine/tests/collect_tests.rs
- baml_language/crates/bex_engine/tests/orchestration.rs
- baml_language/crates/baml_cli/src/lib.rs
- baml_language/crates/bex_engine/tests/tracing.rs
- baml_language/crates/baml_compiler2_hir/src/builder.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- baml_language/crates/bex_engine/tests/registry.rs
- baml_language/crates/bex_engine/tests/gc.rs
- baml_language/crates/baml_lsp2_actions/src/definition.rs
- baml_language/crates/baml_cli/Cargo.toml
- baml_language/crates/bex_engine/tests/cancellation.rs
- baml_language/crates/bex_engine/tests/common/mod.rs
- baml_language/crates/bex_project/src/project.rs
- baml_language/crates/bex_engine/tests/llm_render.rs
- baml_language/crates/baml_compiler2_mir/src/lower.rs
- baml_language/crates/baml_compiler2_tir/src/inference.rs
There was a problem hiding this comment.
♻️ Duplicate comments (2)
baml_language/crates/baml_compiler2_tir/src/builder.rs (2)
4130-4145:⚠️ Potential issue | 🟠 MajorScan every non-empty prefix before treating the chain as an unresolved package path.
Lines 4138-4142 only test the immediate prefix of
item_path. That still breaks longer chains likebaml.argv.map.filter:lookup_value(["argv"], "map")is false, so we reportUnresolvedNameeven though the shorter prefix["argv"]is already a value and the rest should fall back to normal member/method resolution.♻️ Proposed fix
- if !item_path.is_empty() { - let prefix_name = &item_path[item_path.len() - 1]; - let prefix_ns = &item_path[..item_path.len() - 1]; - let prefix_is_value = pkg_items.lookup_value(prefix_ns, prefix_name).is_some(); - if prefix_is_value { - return None; - } - } + for split in 1..=item_path.len() { + let prefix_name = &item_path[split - 1]; + let prefix_ns = &item_path[..split - 1]; + if pkg_items.lookup_value(prefix_ns, prefix_name).is_some() { + return None; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_compiler2_tir/src/builder.rs` around lines 4130 - 4145, The current logic only checks the immediate prefix of item_path before treating it as a package path; update the check so every non-empty prefix of item_path is scanned (not just the last segment) using pkg_items.lookup_value to see if any prefix resolves to a value, and if any does return None to defer to FieldAccess/member resolution; locate the code in builder.rs that uses item_path, prefix_name, prefix_ns and pkg_items.lookup_value and replace the single-prefix check with a loop (or iterator) over all prefixes of item_path to call pkg_items.lookup_value(prefix_ns, prefix_name) for each and bail out if any returns Some.
3237-3291:⚠️ Potential issue | 🔴 CriticalHandle
Definition::Lethere or namespaced free lets still resolve as missing.Line 3239 can return a package-visible
let, but this block only handlesDefinition::Function. As written,pkg.ns.some_let/baml.argvstill falls through toNone, so the new free-let/argv plumbing never becomes visible to TIR resolution. Please add theDefinition::Letpath here, record the corresponding member resolution, and return the let’s type instead of treating it as unresolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_compiler2_tir/src/builder.rs` around lines 3237 - 3291, The lookup_value branch only handles Definition::Function but must also handle Definition::Let: add a match arm for Some(Definition::Let(let_loc)) (similar to the Function branch) where you fetch the item tree and package ns (using baml_compiler2_ppir::file_item_tree and baml_compiler2_hir::file_package like the function case), call self.resolutions.insert(expr_id, crate::inference::MemberResolution::Free { let_loc }), and return a Ty built from the let’s declared type by invoking crate::lower_type_expr::lower_type_expr_in_ns(db, type_expr, pkg_items, &ns_context, generic_params, &mut diags) (or fall back to Ty::Unknown if no type annotation) so namespaced free lets resolve instead of falling through to None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 4130-4145: The current logic only checks the immediate prefix of
item_path before treating it as a package path; update the check so every
non-empty prefix of item_path is scanned (not just the last segment) using
pkg_items.lookup_value to see if any prefix resolves to a value, and if any does
return None to defer to FieldAccess/member resolution; locate the code in
builder.rs that uses item_path, prefix_name, prefix_ns and
pkg_items.lookup_value and replace the single-prefix check with a loop (or
iterator) over all prefixes of item_path to call
pkg_items.lookup_value(prefix_ns, prefix_name) for each and bail out if any
returns Some.
- Around line 3237-3291: The lookup_value branch only handles
Definition::Function but must also handle Definition::Let: add a match arm for
Some(Definition::Let(let_loc)) (similar to the Function branch) where you fetch
the item tree and package ns (using baml_compiler2_ppir::file_item_tree and
baml_compiler2_hir::file_package like the function case), call
self.resolutions.insert(expr_id, crate::inference::MemberResolution::Free {
let_loc }), and return a Ty built from the let’s declared type by invoking
crate::lower_type_expr::lower_type_expr_in_ns(db, type_expr, pkg_items,
&ns_context, generic_params, &mut diags) (or fall back to Ty::Unknown if no type
annotation) so namespaced free lets resolve instead of falling through to None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b160d8ee-6356-424f-9335-c3f4fcd30ef6
📒 Files selected for processing (1)
baml_language/crates/baml_compiler2_tir/src/builder.rs
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
# Conflicts: # baml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snap # baml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snap
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
baml_language/crates/baml_cli/src/commands.rs (1)
1-2:⚠️ Potential issue | 🟡 MinorStale top-of-file TODO.
This file now wires
Run,Describe,Generate,Grep,Test,Format, andLanguageServersubcommands, so the "simplified to only support the LSP command for now" note is inaccurate and should be revised or removed alongside thebaml runaddition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_cli/src/commands.rs` around lines 1 - 2, Update the stale top-of-file TODO comment: remove or rewrite the line that claims the file "has been simplified to only support the LSP command" since the module now wires Run, Describe, Generate, Grep, Test, Format and LanguageServer subcommands (and includes the baml run addition). Locate the comment at the top of the commands.rs file and either delete it or replace it with an accurate summary listing the current supported subcommands (referencing Run, Describe, Generate, Grep, Test, Format, LanguageServer and baml run) so the header reflects the actual state of the codebase.baml_language/crates/baml_cli/src/lib.rs (1)
81-82:⚠️ Potential issue | 🟡 MinorStale doc comment on
run_cli.The doc comment still claims this is "a simplified version that only supports the LSP command," which contradicts the updated crate-level comment and the newly wired
Run/Describe/Generate/Grep/Test/Formatsubcommands. Please update it to reflect the current dispatch surface (includingbaml run).✏️ Proposed fix
-/// Run the CLI with the given arguments. -/// This is a simplified version that only supports the LSP command. +/// Run the CLI with the given arguments. +/// Dispatches to the requested subcommand (e.g. `run`, `describe`, `generate`, +/// `grep`, `test`, `fmt`, `lsp`). pub fn run_cli(argv: Vec<String>) -> Result<ExitCode> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_cli/src/lib.rs` around lines 81 - 82, Update the stale doc comment on the run_cli function to reflect the current CLI dispatch surface: replace the old "only supports the LSP command" text with a concise summary listing the supported subcommands (Run, Describe, Generate, Grep, Test, Format) and mention the top-level "baml run" entry where appropriate; locate the comment immediately above the run_cli function and update its wording to describe the current behavior and available subcommands.
🧹 Nitpick comments (1)
baml_language/crates/bex_engine/src/lib.rs (1)
1170-1170: Add a SAFETY comment on theunsafe { ptr.get() }.Other
ptr.get()sites in this file (e.g. lines 640, 669, 702, 1104, 1116, 1137) carry a// SAFETY: ...note explaining why the raw deref is sound. Please add an equivalent comment here for consistency.Proposed change
.filter_map(|(name, (ptr, kind))| { if !matches!(kind, bex_vm_types::FunctionKind::Bytecode) { return None; } + // SAFETY: ptr is from resolved_function_names, a compile-time object let obj = unsafe { ptr.get() };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_engine/src/lib.rs` at line 1170, Add a SAFETY comment immediately above the unsafe block that calls unsafe { ptr.get() } explaining why dereferencing the raw pointer is sound; specifically state the invariants (e.g., that `ptr` is non-null, properly aligned, points to initialized memory, and outlives this use or is uniquely owned) that guarantee safety in this context and reference the surrounding variables/logic that ensure those invariants (the `ptr` variable and the codepath that constructed/owned it), so it matches the existing comments at other ptr.get() sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@baml_language/crates/baml_cli/src/commands.rs`:
- Around line 1-2: Update the stale top-of-file TODO comment: remove or rewrite
the line that claims the file "has been simplified to only support the LSP
command" since the module now wires Run, Describe, Generate, Grep, Test, Format
and LanguageServer subcommands (and includes the baml run addition). Locate the
comment at the top of the commands.rs file and either delete it or replace it
with an accurate summary listing the current supported subcommands (referencing
Run, Describe, Generate, Grep, Test, Format, LanguageServer and baml run) so the
header reflects the actual state of the codebase.
In `@baml_language/crates/baml_cli/src/lib.rs`:
- Around line 81-82: Update the stale doc comment on the run_cli function to
reflect the current CLI dispatch surface: replace the old "only supports the LSP
command" text with a concise summary listing the supported subcommands (Run,
Describe, Generate, Grep, Test, Format) and mention the top-level "baml run"
entry where appropriate; locate the comment immediately above the run_cli
function and update its wording to describe the current behavior and available
subcommands.
---
Nitpick comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Line 1170: Add a SAFETY comment immediately above the unsafe block that calls
unsafe { ptr.get() } explaining why dereferencing the raw pointer is sound;
specifically state the invariants (e.g., that `ptr` is non-null, properly
aligned, points to initialized memory, and outlives this use or is uniquely
owned) that guarantee safety in this context and reference the surrounding
variables/logic that ensure those invariants (the `ptr` variable and the
codepath that constructed/owned it), so it matches the existing comments at
other ptr.get() sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7cf324fa-70ad-48bd-9426-c658cdbfbaf3
⛔ Files ignored due to path filters (7)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_tests/snapshots/__baml_std__/baml_tests____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/__baml_std__/baml_tests____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/__baml_std__/baml_tests____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
baml_language/crates/baml_builtins2/baml_std/baml/ns_sys/sys.bamlbaml_language/crates/baml_cli/Cargo.tomlbaml_language/crates/baml_cli/src/commands.rsbaml_language/crates/baml_cli/src/lib.rsbaml_language/crates/baml_cli/src/run_command.rsbaml_language/crates/baml_cli/src/test_command.rsbaml_language/crates/baml_tests/src/engine.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/build_request.rsbaml_language/crates/bex_engine/tests/cancellation.rsbaml_language/crates/bex_engine/tests/collect_tests.rsbaml_language/crates/bex_engine/tests/common/mod.rsbaml_language/crates/bex_engine/tests/concurrent.rsbaml_language/crates/bex_engine/tests/gc.rsbaml_language/crates/bex_engine/tests/llm_render.rsbaml_language/crates/bex_engine/tests/orchestration.rsbaml_language/crates/bex_engine/tests/registry.rsbaml_language/crates/bex_engine/tests/tracing.rsbaml_language/crates/bex_project/src/project.rsbaml_language/crates/bex_vm/src/package_baml/sys.rsbaml_language/crates/bex_vm/src/vm.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
baml_language/crates/bex_engine/src/lib.rs (1)
1151-1180:⚠️ Potential issue | 🟠 MajorUser-callable predicate still split between
function_existsanduser_functions.
function_existsreturns true for any resolved symbol (including$init,$init_test,user.Foo$render_prompt, native/builtin names), whileuser_functionsfilters only onFunctionKind::Bytecodeand strips auser.prefix. That means:
baml run --function $init/$init_test/testing.TestCollector.newwould passfunction_existsvalidation but never appear in--list.user_functions()will happily emit synthetic helpers likeFoo$render_prompt(Bytecode,user.-prefixed) as public entries, since there is no$filter.Consider a single
is_user_callable(name, kind)predicate (requiresuser.prefix,Bytecodekind, and no$in the suffix) and share it between both methods so CLI--function, suggestions, and--listall agree on the same surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_engine/src/lib.rs` around lines 1151 - 1180, The current behavior splits user-callable logic between function_exists and user_functions causing mismatches; introduce a shared is_user_callable(name: &str, kind: &bex_vm_types::FunctionKind) -> bool that returns true only if the symbol has the "user." prefix, matches FunctionKind::Bytecode, and its unprefixed suffix does not contain '$' (to exclude synthetic helpers), then use this predicate inside resolve_function_name-based function_exists (or when checking the resolved kind) and inside user_functions when filtering resolved_function_names so CLI --function, suggestions, and --list use the same criteria.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 1151-1180: The current behavior splits user-callable logic between
function_exists and user_functions causing mismatches; introduce a shared
is_user_callable(name: &str, kind: &bex_vm_types::FunctionKind) -> bool that
returns true only if the symbol has the "user." prefix, matches
FunctionKind::Bytecode, and its unprefixed suffix does not contain '$' (to
exclude synthetic helpers), then use this predicate inside
resolve_function_name-based function_exists (or when checking the resolved kind)
and inside user_functions when filtering resolved_function_names so CLI
--function, suggestions, and --list use the same criteria.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d87532f5-b471-4852-99f2-808a4b0b09dc
⛔ Files ignored due to path filters (4)
baml_language/crates/baml_tests/snapshots/__baml_std__/baml_tests____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/__baml_std__/baml_tests____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
baml_language/crates/baml_cli/src/commands.rsbaml_language/crates/baml_cli/src/lib.rsbaml_language/crates/baml_cli/src/run_command.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/collect_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/crates/baml_cli/src/lib.rs
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #3377 |
Summary by CodeRabbit
baml runto execute functions or standalone .baml scripts (namespace/function/file)argv()API