Repository navigation
tests: update sdk_tests to support nodejs and python - #3558
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRestructures sdk_tests into a shared build crate with generator-target crates (python_pydantic2, nodejs), moves bridge/codegen under sdks/, adds codegen stubs, updates Cargo workspace and CI proto/workflow paths, and converts many fixtures to per-fixture generated tests (adds/renames RoundTrip* helpers). ChangesSDK Test Infrastructure and Layout Consolidation
Estimated code review effort: Possibly related PRs:
Poem:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48d0b0d65e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Binary size checks passed✅ 7 passed
Generated by |
Merging this PR will degrade performance by 11.85%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | vm_array_push_50k |
8.9 ms | 10.3 ms | -13.1% |
| ❌ | WallTime | vm_class_create_50k |
11.3 ms | 12.8 ms | -11.79% |
| ❌ | WallTime | vm_mixed_ops |
7.7 ms | 11.2 ms | -30.69% |
| ❌ | WallTime | vm_fib_20 |
5.1 ms | 5.8 ms | -11.62% |
| ⚡ | WallTime | vm_string_concat_5k |
31.3 ms | 27.6 ms | +13.35% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing sam/nodejs-setup (484e47e) with canary (1652b45)
sxlijin
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/Cargo.toml`:
- Around line 5-6: The workspace currently lists "sdks/nodejs/bridge_nodejs"
which pulls that package into generic CI runs and breaks Windows; update
Cargo.toml's [workspace] so that "sdks/nodejs/bridge_nodejs" is not included in
the generic workspace (either remove that entry from the workspace.members list
or add it to workspace.exclude = ["sdks/nodejs/bridge_nodejs"]), or
alternatively ensure the package is only added in the Node-specific CI by moving
the "sdks/nodejs/bridge_nodejs" entry out of the main members list and into the
NodeJS-specific workspace configuration used by your Node CI; this prevents
cargo test --no-run --all-features from compiling the Node bridge on Windows.
In `@baml_language/sdk_tests/build/src/nodejs.rs`:
- Around line 141-155: run_all() currently unconditionally calls
pnpm_install(...) which panics if pnpm is unavailable; change run_all() to first
check for pnpm on PATH and skip calling pnpm_install for all fixtures when it's
missing, emitting a clear warning instead of failing the build. Implement the
check by attempting to run "pnpm --version" (or using which::which("pnpm")) and
only loop calling pnpm_install when that succeeds; also change pnpm_install to
return a Result and have run_all handle and log non-fatal failures
(skip/continue) rather than letting spawn/exit errors abort the process.
In `@baml_language/sdk_tests/build/src/python_pydantic2.rs`:
- Around line 112-127: The uv sync step currently panics on spawn failure or
non-zero exit inside uv_sync; change uv_sync to treat missing/failed `uv` as
non-fatal by catching Command::new("uv") spawn errors and the child process exit
status, logging a warning (including the error/output) and returning Ok(()) or a
skipped result instead of unwrap_or_else/panic! or assert!, and update the loop
that calls uv_sync (for fixture in &fixtures { uv_sync(...) }) to continue on
those non-fatal results so the build script skips syncing when `uv` isn't
available.
In `@baml_language/sdk_tests/README.md`:
- Around line 118-120: The README currently claims the scaffold emits “the four
`#[test]` fns” which conflicts with the Node.js target that runs three checks;
update the wording in the description of OUT_DIR/<generator>_tests.rs (the
sentence referencing `mod <fixture> { … }`, `#[test]` fns and the toolchain
commands) to be target-agnostic—either state “the `#[test]` functions for each
fixture (number varies by target)” or list per-target counts, so the text no
longer asserts “the four `#[test]` fns” universally.
In `@baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs`:
- Around line 23-25: The unimplemented!() in codegen_nodejs::to_source_code is
panicking during CI; replace it with a non-panicking stub that matches the
function signature (do not call unimplemented! or panic!), e.g. return a
harmless value or a propagated error (Result/Option) instead of panicking so
builds/tests don't abort; update the body of to_source_code in lib.rs to return
a placeholder source or an Err-compatible value and ensure callers can handle
that return without causing a panic.
🪄 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: 863ccdcf-488f-4fcd-9af3-bd78975ad8be
⛔ Files ignored due to path filters (14)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_inbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_outbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/go.sumis excluded by!**/*.sumbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/sdks/nodejs/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (138)
.github/workflows/cargo-tests.reusable.yaml.github/workflows/ci.yamlbaml_language/Cargo.tomlbaml_language/crates/bridge_ctypes/README.mdbaml_language/sdk_tests/README.mdbaml_language/sdk_tests/build/Cargo.tomlbaml_language/sdk_tests/build/src/lib.rsbaml_language/sdk_tests/build/src/nodejs.rsbaml_language/sdk_tests/build/src/python_pydantic2.rsbaml_language/sdk_tests/crates/basic_type_shapes/Cargo.tomlbaml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_complex/types.bamlbaml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_defaults/types.bamlbaml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_literals/types.bamlbaml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_maps/types.bamlbaml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_ty_variants/types.bamlbaml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_unions/types.bamlbaml_language/sdk_tests/crates/basic_type_shapes/build.rsbaml_language/sdk_tests/crates/basic_type_shapes/customizable/test_main.pybaml_language/sdk_tests/crates/basic_type_shapes/src/lib.rsbaml_language/sdk_tests/crates/docstrings_etc/Cargo.tomlbaml_language/sdk_tests/crates/docstrings_etc/build.rsbaml_language/sdk_tests/crates/docstrings_etc/src/lib.rsbaml_language/sdk_tests/crates/llm_functions/Cargo.tomlbaml_language/sdk_tests/crates/llm_functions/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/crates/llm_functions/build.rsbaml_language/sdk_tests/crates/llm_functions/src/lib.rsbaml_language/sdk_tests/crates/nodejs/Cargo.tomlbaml_language/sdk_tests/crates/nodejs/build.rsbaml_language/sdk_tests/crates/nodejs/customizable/docstrings_etc/main.test.tsbaml_language/sdk_tests/crates/nodejs/customizable/llm_functions/main.test.tsbaml_language/sdk_tests/crates/nodejs/customizable/type_shapes/main.test.tsbaml_language/sdk_tests/crates/nodejs/src/lib.rsbaml_language/sdk_tests/crates/python_pydantic2/Cargo.tomlbaml_language/sdk_tests/crates/python_pydantic2/build.rsbaml_language/sdk_tests/crates/python_pydantic2/customizable/docstrings_etc/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/llm_functions/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/llm_functions/test_streaming_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_generic_method.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/src/lib.rsbaml_language/sdk_tests/crates/type_shapes/Cargo.tomlbaml_language/sdk_tests/crates/type_shapes/build.rsbaml_language/sdk_tests/crates/type_shapes/src/lib.rsbaml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_docs/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_ipsum/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_ipsum/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases_consumer/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_class_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_enums/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_forward_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/methods.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_ipsum/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lists/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_literals/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/cross_uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/streams.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_maps/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_media/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_optional/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_primitives/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_recursion/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/README.mdbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_a/ns_b/ns_c/ns_d/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_buzz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_unions/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_void/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/root.bamlbaml_language/sdks/go/bridge_go/build.shbaml_language/sdks/go/bridge_go/cffi/bridge.hbaml_language/sdks/go/bridge_go/cffi/exports.gobaml_language/sdks/go/bridge_go/cffi/lib.gobaml_language/sdks/go/bridge_go/cffi/types.gobaml_language/sdks/go/bridge_go/go.modbaml_language/sdks/go/bridge_go/pkg/callbacks.gobaml_language/sdks/go/bridge_go/pkg/errors.gobaml_language/sdks/go/bridge_go/pkg/handle.gobaml_language/sdks/go/bridge_go/pkg/init_callbacks.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/go/bridge_go/pkg/runtime.gobaml_language/sdks/go/bridge_go/pkg/tests/call_function_test.gobaml_language/sdks/go/bridge_go/pkg/tests/engine_test.gobaml_language/sdks/go/bridge_go/pkg/tests/handle_test.gobaml_language/sdks/go/bridge_go/pkg/types.gobaml_language/sdks/go/codegen_go/Cargo.tomlbaml_language/sdks/go/codegen_go/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/Cargo.tomlbaml_language/sdks/nodejs/bridge_nodejs/build.rsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.tsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.jsbaml_language/sdks/nodejs/bridge_nodejs/errors.d.tsbaml_language/sdks/nodejs/bridge_nodejs/errors.jsbaml_language/sdks/nodejs/bridge_nodejs/index.d.tsbaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/package.jsonbaml_language/sdks/nodejs/bridge_nodejs/protobaml_language/sdks/nodejs/bridge_nodejs/proto.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rsbaml_language/sdks/nodejs/bridge_nodejs/src/errors.rsbaml_language/sdks/nodejs/bridge_nodejs/src/handle.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/collector.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/host_span_manager.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/mod.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/run_jest.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tsconfig.jsonbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/generated-header.txtbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.jsbaml_language/sdks/nodejs/codegen_nodejs/Cargo.tomlbaml_language/sdks/nodejs/codegen_nodejs/src/lib.rsmise.tomlpnpm-workspace.yaml
💤 Files with no reviewable changes (21)
- baml_language/sdk_tests/crates/type_shapes/Cargo.toml
- baml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_literals/types.baml
- baml_language/sdk_tests/crates/type_shapes/build.rs
- baml_language/sdk_tests/crates/type_shapes/src/lib.rs
- baml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_ty_variants/types.baml
- baml_language/sdk_tests/crates/llm_functions/src/lib.rs
- baml_language/sdk_tests/crates/basic_type_shapes/src/lib.rs
- baml_language/sdk_tests/crates/llm_functions/Cargo.toml
- baml_language/sdk_tests/crates/docstrings_etc/build.rs
- baml_language/sdk_tests/crates/docstrings_etc/Cargo.toml
- baml_language/sdk_tests/crates/basic_type_shapes/Cargo.toml
- baml_language/sdk_tests/crates/llm_functions/build.rs
- baml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_maps/types.baml
- baml_language/sdk_tests/crates/docstrings_etc/src/lib.rs
- baml_language/sdk_tests/crates/llm_functions/baml_src/ns_lorem/types.baml
- baml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_unions/types.baml
- baml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_defaults/types.baml
- baml_language/sdk_tests/crates/basic_type_shapes/build.rs
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.baml
- baml_language/sdk_tests/crates/basic_type_shapes/baml_src/ns_complex/types.baml
- baml_language/sdk_tests/crates/basic_type_shapes/customizable/test_main.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/build/src/lib.rs (1)
200-218:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWatch directories too for
cargo:rerun-if-changed.
watch_diremitscargo:rerun-if-changedfor only the paths returned bywalk_files, butwalk_filescurrently collects files (not directories). As a result, adding a new file under an already-existing subdirectory won’t trigger the build script rerun. Emit directory paths as well so Cargo’s recursive directory tracking catches new additions.Suggested patch
pub fn walk_files(dir: &Path) -> Vec<PathBuf> { let mut out = Vec::new(); if let Ok(entries) = fs::read_dir(dir) { for entry in entries.flatten() { let path = entry.path(); if path.is_file() { out.push(path); } else if path.is_dir() { + out.push(path.clone()); out.extend(walk_files(&path)); } } } out }🤖 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/build/src/lib.rs` around lines 200 - 218, watch_dir only emits cargo:rerun-if-changed for file paths because walk_files returns files only; to ensure Cargo notices new files added to existing subdirectories, include directory paths as well. Update walk_files (or adjust watch_dir) so it also pushes each directory PathBuf (e.g., when entry.path().is_dir() push path before recursing) or have watch_dir emit the directory path for each visited directory in addition to files; keep the existing behavior of returning an empty Vec when dir is missing and preserve recursion in walk_files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/sdk_tests/crates/python_pydantic2/build.rs`:
- Around line 6-8: This PR needs explicit evidence that the Rust library tests
were run with `cargo test --lib` for the modified crate; run `cargo test --lib`
(or `cargo nextest` with equivalent coverage) in the
sdk_tests/crates/python_pydantic2 workspace and capture the test output for the
`sdk_test_python_pydantic2` crate (or a CI job/log URL) and attach it to the PR,
referencing the build entry point `build.rs` and the call to
`sdk_test_build::python_pydantic2::run_all()` so reviewers can confirm the
library tests passed. Ensure the posted output includes the passing summary
lines from `cargo test --lib` for that crate.
In `@baml_language/sdk_tests/README.md`:
- Around line 38-40: The example in README.md uses a stale test selector
"sync_only" for the sdk_test_python_pydantic2 package; update the command to
invoke an actual check emitted by that scaffold (e.g., "ruff", "pyright", or
"pytest") so it selects tests correctly — replace "sync_only" in the shown
command invocation for sdk_test_python_pydantic2 with one of "ruff", "pyright",
or "pytest" (or another current test name) and verify the `cargo test -p
sdk_test_python_pydantic2 ...` example runs the intended check.
In `@baml_language/sdks/go/codegen_go/src/lib.rs`:
- Around line 22-24: Replace the hard panic in the public function
to_source_code (in lib.rs of codegen_go) with a non‑panicking stub that matches
the function's signature: if to_source_code returns a Result, return an Err
containing a descriptive error indicating "Go emitter not implemented
(placeholder)"; if it returns a String/owned value, return a clear placeholder
string like "Go emitter not implemented"; ensure the returned value is
descriptive for callers and avoid using panicking macros
(unimplemented!/panic!).
---
Outside diff comments:
In `@baml_language/sdk_tests/build/src/lib.rs`:
- Around line 200-218: watch_dir only emits cargo:rerun-if-changed for file
paths because walk_files returns files only; to ensure Cargo notices new files
added to existing subdirectories, include directory paths as well. Update
walk_files (or adjust watch_dir) so it also pushes each directory PathBuf (e.g.,
when entry.path().is_dir() push path before recursing) or have watch_dir emit
the directory path for each visited directory in addition to files; keep the
existing behavior of returning an empty Vec when dir is missing and preserve
recursion in walk_files.
🪄 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: 687fd9f5-4e9c-483d-b923-d8675564f120
⛔ Files ignored due to path filters (14)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_inbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_outbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/go.sumis excluded by!**/*.sumbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/sdks/nodejs/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (127)
.github/workflows/cargo-tests.reusable.yaml.github/workflows/ci.yamlbaml_language/Cargo.tomlbaml_language/crates/bridge_ctypes/README.mdbaml_language/sdk_tests/README.mdbaml_language/sdk_tests/build/Cargo.tomlbaml_language/sdk_tests/build/src/lib.rsbaml_language/sdk_tests/build/src/nodejs.rsbaml_language/sdk_tests/build/src/python_pydantic2.rsbaml_language/sdk_tests/crates/docstrings_etc/Cargo.tomlbaml_language/sdk_tests/crates/docstrings_etc/build.rsbaml_language/sdk_tests/crates/docstrings_etc/src/lib.rsbaml_language/sdk_tests/crates/llm_functions/Cargo.tomlbaml_language/sdk_tests/crates/llm_functions/build.rsbaml_language/sdk_tests/crates/llm_functions/src/lib.rsbaml_language/sdk_tests/crates/nodejs/Cargo.tomlbaml_language/sdk_tests/crates/nodejs/build.rsbaml_language/sdk_tests/crates/nodejs/customizable/docstrings_etc/main.test.tsbaml_language/sdk_tests/crates/nodejs/customizable/llm_functions/main.test.tsbaml_language/sdk_tests/crates/nodejs/customizable/type_shapes/main.test.tsbaml_language/sdk_tests/crates/nodejs/src/lib.rsbaml_language/sdk_tests/crates/python_pydantic2/Cargo.tomlbaml_language/sdk_tests/crates/python_pydantic2/build.rsbaml_language/sdk_tests/crates/python_pydantic2/customizable/docstrings_etc/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/llm_functions/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/llm_functions/test_streaming_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_generic_method.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/src/lib.rsbaml_language/sdk_tests/crates/type_shapes/Cargo.tomlbaml_language/sdk_tests/crates/type_shapes/build.rsbaml_language/sdk_tests/crates/type_shapes/src/lib.rsbaml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_docs/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_ipsum/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_ipsum/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases_consumer/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_class_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_enums/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_forward_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/methods.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_ipsum/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lists/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_literals/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/cross_uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/streams.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_maps/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_media/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_optional/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_primitives/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_recursion/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/README.mdbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_a/ns_b/ns_c/ns_d/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_buzz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_unions/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_void/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/root.bamlbaml_language/sdks/go/bridge_go/build.shbaml_language/sdks/go/bridge_go/cffi/bridge.hbaml_language/sdks/go/bridge_go/cffi/exports.gobaml_language/sdks/go/bridge_go/cffi/lib.gobaml_language/sdks/go/bridge_go/cffi/types.gobaml_language/sdks/go/bridge_go/go.modbaml_language/sdks/go/bridge_go/pkg/callbacks.gobaml_language/sdks/go/bridge_go/pkg/errors.gobaml_language/sdks/go/bridge_go/pkg/handle.gobaml_language/sdks/go/bridge_go/pkg/init_callbacks.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/go/bridge_go/pkg/runtime.gobaml_language/sdks/go/bridge_go/pkg/tests/call_function_test.gobaml_language/sdks/go/bridge_go/pkg/tests/engine_test.gobaml_language/sdks/go/bridge_go/pkg/tests/handle_test.gobaml_language/sdks/go/bridge_go/pkg/types.gobaml_language/sdks/go/codegen_go/Cargo.tomlbaml_language/sdks/go/codegen_go/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/Cargo.tomlbaml_language/sdks/nodejs/bridge_nodejs/build.rsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.tsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.jsbaml_language/sdks/nodejs/bridge_nodejs/errors.d.tsbaml_language/sdks/nodejs/bridge_nodejs/errors.jsbaml_language/sdks/nodejs/bridge_nodejs/index.d.tsbaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/package.jsonbaml_language/sdks/nodejs/bridge_nodejs/protobaml_language/sdks/nodejs/bridge_nodejs/proto.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rsbaml_language/sdks/nodejs/bridge_nodejs/src/errors.rsbaml_language/sdks/nodejs/bridge_nodejs/src/handle.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/collector.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/host_span_manager.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/mod.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/run_jest.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tsconfig.jsonbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/generated-header.txtbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.jsbaml_language/sdks/nodejs/codegen_nodejs/Cargo.tomlbaml_language/sdks/nodejs/codegen_nodejs/src/lib.rsmise.tomlpnpm-workspace.yaml
💤 Files with no reviewable changes (38)
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_lorem/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_a/ns_b/ns_c/ns_d/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/root.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_buzz/ns_foo/types.baml
- baml_language/sdk_tests/crates/docstrings_etc/Cargo.toml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_forward_refs/types.baml
- baml_language/sdk_tests/crates/type_shapes/Cargo.toml
- baml_language/sdk_tests/crates/llm_functions/src/lib.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/cross_uses.baml
- baml_language/sdk_tests/crates/llm_functions/Cargo.toml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_foo/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_recursion/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_primitives/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_foo/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_media/types.baml
- baml_language/sdk_tests/crates/docstrings_etc/src/lib.rs
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.baml
- baml_language/sdk_tests/crates/llm_functions/build.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_class_refs/types.baml
- baml_language/sdk_tests/crates/type_shapes/build.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_ipsum/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_optional/types.baml
- baml_language/sdk_tests/crates/type_shapes/src/lib.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/uses.baml
- baml_language/sdk_tests/crates/docstrings_etc/build.rs
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_enums/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lists/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_literals/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/streams.baml
- baml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_generic_method.py
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/methods.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_maps/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_unions/types.baml
✅ Files skipped from review due to trivial changes (7)
- baml_language/sdks/go/bridge_go/build.sh
- pnpm-workspace.yaml
- baml_language/sdk_tests/crates/python_pydantic2/src/lib.rs
- baml_language/sdks/nodejs/codegen_nodejs/Cargo.toml
- .github/workflows/cargo-tests.reusable.yaml
- mise.toml
- baml_language/sdk_tests/crates/nodejs/Cargo.toml
Comment on lines
+22
to
+24
| unimplemented!( | ||
| "codegen_go::to_source_code is a stub — the Go emitter has not been implemented yet" | ||
| ); |
Contributor
There was a problem hiding this comment.
Avoid hard panic in public to_source_code API.
unimplemented!() will abort callers at runtime; prefer a non-panicking placeholder until the emitter lands.
Proposed minimal non-panicking stub
pub fn to_source_code(
_pool: &SymbolPool,
_user_baml_files: &[UserBamlFile],
_naming_convention: NamingConvention,
) -> HashMap<PathBuf, String> {
- unimplemented!(
- "codegen_go::to_source_code is a stub — the Go emitter has not been implemented yet"
- );
+ HashMap::new()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| unimplemented!( | |
| "codegen_go::to_source_code is a stub — the Go emitter has not been implemented yet" | |
| ); | |
| pub fn to_source_code( | |
| _pool: &SymbolPool, | |
| _user_baml_files: &[UserBamlFile], | |
| _naming_convention: NamingConvention, | |
| ) -> HashMap<PathBuf, String> { | |
| HashMap::new() | |
| } |
🤖 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/go/codegen_go/src/lib.rs` around lines 22 - 24, Replace
the hard panic in the public function to_source_code (in lib.rs of codegen_go)
with a non‑panicking stub that matches the function's signature: if
to_source_code returns a Result, return an Err containing a descriptive error
indicating "Go emitter not implemented (placeholder)"; if it returns a
String/owned value, return a clear placeholder string like "Go emitter not
implemented"; ensure the returned value is descriptive for callers and avoid
using panicking macros (unimplemented!/panic!).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
baml_language/sdks/go/codegen_go/src/lib.rs (1)
22-24:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAvoid hard panic in public
to_source_codeAPI.The
unimplemented!()macro causes a hard panic at runtime. ReturnHashMap::new()instead to allow callers to handle an empty output gracefully until the Go emitter is implemented.🛠️ Proposed fix
pub fn to_source_code( _pool: &SymbolPool, _user_baml_files: &[UserBamlFile], _naming_convention: NamingConvention, ) -> HashMap<PathBuf, String> { - unimplemented!( - "codegen_go::to_source_code is a stub — the Go emitter has not been implemented yet" - ); + HashMap::new() }🤖 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/go/codegen_go/src/lib.rs` around lines 22 - 24, The public function to_source_code in lib.rs currently uses unimplemented!() which panics; change it to return an empty HashMap instead so callers receive an empty result rather than a runtime panic. Update the body of to_source_code to construct and return HashMap::new() (ensuring the function's return type matches) and remove the unimplemented!() call; add or keep the necessary std::collections::HashMap import if missing. Ensure no panic paths remain in to_source_code so callers can handle missing emitter output gracefully.baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs (1)
23-25:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace the panic stub with a non-panicking placeholder.
Line 23 currently panics unconditionally, which aborts every caller path that invokes Node codegen. Please return a harmless placeholder until the emitter is implemented.
Proposed minimal fix
pub fn to_source_code( _pool: &SymbolPool, _user_baml_files: &[UserBamlFile], _naming_convention: NamingConvention, ) -> HashMap<PathBuf, String> { - unimplemented!( - "codegen_nodejs::to_source_code is a stub — the Node.js emitter has not been implemented yet" - ); + HashMap::new() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs` around lines 23 - 25, The to_source_code implementation currently panics via unimplemented! which aborts callers; replace the panic in codegen_nodejs::to_source_code with a non-panicking placeholder return (e.g., a harmless placeholder string like "/* Node.js emitter not implemented */" or a Result wrapping that string), matching the function's declared return type, remove the unimplemented! macro, and add a short TODO comment indicating the real emitter must be implemented later.
🤖 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 @.github/workflows/cargo-tests.reusable.yaml:
- Around line 323-325: Add an explicit "cargo test --lib" invocation to the Rust
test step so CI runs library tests in addition to nextest; modify the step named
"Run sdk tests" (the command currently using "cargo nextest run --all-features
-E 'package(/^sdk_test_/)'") to run "cargo test --lib" in the baml_language
working directory before invoking nextest (or add a separate step that runs
"cargo test --lib" with working-directory set to baml_language).
In `@baml_language/sdk_tests/README.md`:
- Around line 113-115: Update the README text that currently states "Symlinks
each file in `customizable/<fixture>/` into `generated/<fixture>/` (falls back
to copy on Windows without Developer Mode)" to be target-specific: change the
description for Node.js fixtures to say that overlays in
`customizable/<fixture>/` are copied into `generated/<fixture>/` on all
platforms (no symlink), while keeping the original symlink-with-Windows-fallback
wording for other targets; reference the paths `customizable/<fixture>/` and
`generated/<fixture>/` and the "Node.js" target in the revised sentence so
readers can clearly see the different behavior per target.
---
Duplicate comments:
In `@baml_language/sdks/go/codegen_go/src/lib.rs`:
- Around line 22-24: The public function to_source_code in lib.rs currently uses
unimplemented!() which panics; change it to return an empty HashMap instead so
callers receive an empty result rather than a runtime panic. Update the body of
to_source_code to construct and return HashMap::new() (ensuring the function's
return type matches) and remove the unimplemented!() call; add or keep the
necessary std::collections::HashMap import if missing. Ensure no panic paths
remain in to_source_code so callers can handle missing emitter output
gracefully.
In `@baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs`:
- Around line 23-25: The to_source_code implementation currently panics via
unimplemented! which aborts callers; replace the panic in
codegen_nodejs::to_source_code with a non-panicking placeholder return (e.g., a
harmless placeholder string like "/* Node.js emitter not implemented */" or a
Result wrapping that string), matching the function's declared return type,
remove the unimplemented! macro, and add a short TODO comment indicating the
real emitter must be implemented later.
🪄 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: f6c4c3b6-bef2-472a-9f3f-2511bd8542d6
⛔ Files ignored due to path filters (14)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_inbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_outbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/go.sumis excluded by!**/*.sumbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/sdks/nodejs/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (127)
.github/workflows/cargo-tests.reusable.yaml.github/workflows/ci.yamlbaml_language/Cargo.tomlbaml_language/crates/bridge_ctypes/README.mdbaml_language/sdk_tests/README.mdbaml_language/sdk_tests/build/Cargo.tomlbaml_language/sdk_tests/build/src/lib.rsbaml_language/sdk_tests/build/src/nodejs.rsbaml_language/sdk_tests/build/src/python_pydantic2.rsbaml_language/sdk_tests/crates/docstrings_etc/Cargo.tomlbaml_language/sdk_tests/crates/docstrings_etc/build.rsbaml_language/sdk_tests/crates/docstrings_etc/src/lib.rsbaml_language/sdk_tests/crates/llm_functions/Cargo.tomlbaml_language/sdk_tests/crates/llm_functions/build.rsbaml_language/sdk_tests/crates/llm_functions/src/lib.rsbaml_language/sdk_tests/crates/nodejs/Cargo.tomlbaml_language/sdk_tests/crates/nodejs/build.rsbaml_language/sdk_tests/crates/nodejs/customizable/docstrings_etc/main.test.tsbaml_language/sdk_tests/crates/nodejs/customizable/llm_functions/main.test.tsbaml_language/sdk_tests/crates/nodejs/customizable/type_shapes/main.test.tsbaml_language/sdk_tests/crates/nodejs/src/lib.rsbaml_language/sdk_tests/crates/python_pydantic2/Cargo.tomlbaml_language/sdk_tests/crates/python_pydantic2/build.rsbaml_language/sdk_tests/crates/python_pydantic2/customizable/docstrings_etc/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/llm_functions/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/llm_functions/test_streaming_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_generic_method.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/src/lib.rsbaml_language/sdk_tests/crates/type_shapes/Cargo.tomlbaml_language/sdk_tests/crates/type_shapes/build.rsbaml_language/sdk_tests/crates/type_shapes/src/lib.rsbaml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_docs/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_ipsum/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_ipsum/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases_consumer/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_class_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_enums/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_forward_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/methods.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_ipsum/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lists/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_literals/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/cross_uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/streams.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_maps/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_media/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_optional/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_primitives/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_recursion/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/README.mdbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_a/ns_b/ns_c/ns_d/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_buzz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_unions/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_void/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/root.bamlbaml_language/sdks/go/bridge_go/build.shbaml_language/sdks/go/bridge_go/cffi/bridge.hbaml_language/sdks/go/bridge_go/cffi/exports.gobaml_language/sdks/go/bridge_go/cffi/lib.gobaml_language/sdks/go/bridge_go/cffi/types.gobaml_language/sdks/go/bridge_go/go.modbaml_language/sdks/go/bridge_go/pkg/callbacks.gobaml_language/sdks/go/bridge_go/pkg/errors.gobaml_language/sdks/go/bridge_go/pkg/handle.gobaml_language/sdks/go/bridge_go/pkg/init_callbacks.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/go/bridge_go/pkg/runtime.gobaml_language/sdks/go/bridge_go/pkg/tests/call_function_test.gobaml_language/sdks/go/bridge_go/pkg/tests/engine_test.gobaml_language/sdks/go/bridge_go/pkg/tests/handle_test.gobaml_language/sdks/go/bridge_go/pkg/types.gobaml_language/sdks/go/codegen_go/Cargo.tomlbaml_language/sdks/go/codegen_go/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/Cargo.tomlbaml_language/sdks/nodejs/bridge_nodejs/build.rsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.tsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.jsbaml_language/sdks/nodejs/bridge_nodejs/errors.d.tsbaml_language/sdks/nodejs/bridge_nodejs/errors.jsbaml_language/sdks/nodejs/bridge_nodejs/index.d.tsbaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/package.jsonbaml_language/sdks/nodejs/bridge_nodejs/protobaml_language/sdks/nodejs/bridge_nodejs/proto.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rsbaml_language/sdks/nodejs/bridge_nodejs/src/errors.rsbaml_language/sdks/nodejs/bridge_nodejs/src/handle.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/collector.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/host_span_manager.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/mod.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/run_jest.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tsconfig.jsonbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/generated-header.txtbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.jsbaml_language/sdks/nodejs/codegen_nodejs/Cargo.tomlbaml_language/sdks/nodejs/codegen_nodejs/src/lib.rsmise.tomlpnpm-workspace.yaml
💤 Files with no reviewable changes (38)
- baml_language/sdk_tests/crates/type_shapes/Cargo.toml
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_a/ns_b/ns_c/ns_d/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/root.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_buzz/ns_foo/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_foo/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_class_refs/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/cross_uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_lorem/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_recursion/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_media/types.baml
- baml_language/sdk_tests/crates/llm_functions/Cargo.toml
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_optional/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_ipsum/uses.baml
- baml_language/sdk_tests/crates/type_shapes/build.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/methods.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_forward_refs/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_enums/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_foo/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_literals/types.baml
- baml_language/sdk_tests/crates/docstrings_etc/Cargo.toml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases/types.baml
- baml_language/sdk_tests/crates/docstrings_etc/src/lib.rs
- baml_language/sdk_tests/crates/llm_functions/src/lib.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_primitives/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/uses.baml
- baml_language/sdk_tests/crates/type_shapes/src/lib.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/streams.baml
- baml_language/sdk_tests/crates/llm_functions/build.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lists/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_maps/types.baml
- baml_language/sdk_tests/crates/docstrings_etc/build.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_unions/types.baml
- baml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_generic_method.py
✅ Files skipped from review due to trivial changes (7)
- baml_language/sdk_tests/crates/nodejs/customizable/docstrings_etc/main.test.ts
- baml_language/sdk_tests/crates/python_pydantic2/Cargo.toml
- mise.toml
- baml_language/crates/bridge_ctypes/README.md
- pnpm-workspace.yaml
- baml_language/sdk_tests/crates/nodejs/customizable/type_shapes/main.test.ts
- baml_language/sdks/go/codegen_go/Cargo.toml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
.github/workflows/cargo-tests.reusable.yaml (1)
319-325:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an explicit
cargo test --libstep to this Rust CI flow.This workflow currently compiles tests and runs nextest, but it still skips the required direct lib-test invocation. Please add
cargo test --lib --all-featuresinbaml_languagebefore/alongside the sdk nextest step.As per coding guidelines: "Always run
cargo test --libif you changed any Rust 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 @.github/workflows/cargo-tests.reusable.yaml around lines 319 - 325, Add an explicit lib test step to the CI by inserting a new GitHub Actions step (e.g., name it "Run lib tests") that runs `cargo test --lib --all-features` with working-directory set to baml_language, and place it before the existing "Run sdk tests" step (the step that runs `cargo nextest run --all-features -E 'package(/^sdk_test_/)'`) so the workflow performs the direct lib-test invocation required by the guidelines.baml_language/sdk_tests/README.md (2)
22-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNode.js check list is stale (
sync_onlyis no longer emitted).The Node scaffold currently generates
tscandjesttests; documentingsync_onlyhere will mislead test selection and contributor expectations.🤖 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 22 - 24, Update the README entry for sdk_test_nodejs to remove the stale reference to the `sync_only` check and reflect the current Node scaffold: mention only the two checks emitted (`tsc` via `pnpm exec tsc --noEmit` and `jest` via `pnpm exec jest`) in the `sdk_test_nodejs` description so contributors see the correct test selection; ensure the symbol `sdk_test_nodejs` remains and `sync_only` is no longer listed.
113-120:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win“How It Works” currently describes Python behavior as universal.
This section says overlays are symlinked, four tests are emitted, and CI pre-warms with
sync_only; that does not match the current Node target flow. Please make this section target-specific (or explicitly split Python vs Node behavior).Also applies to: 134-139
🤖 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 113 - 120, The "How It Works" section currently treats Python behavior as universal; update the README's "How It Works" paragraph(s) to explicitly separate Python and Node flows (or make behavior target-specific): state that for Python fixtures overlays are symlinked into generated/<fixture>/ with fallback copy on Windows, pyproject.toml is written per-fixture, and OUT_DIR/<generator>_tests.rs emits the four #[test] fns; and for the Node target describe the differing overlay/copy semantics, any different generated manifest (e.g., package.json) behavior, and the different test-generation or CI sync_only pre-warm behavior referenced earlier (also update the similar text at the later block around lines 134-139). Ensure the README mentions both pyproject.toml and package.json where appropriate and clarifies which behavior applies to which target.baml_language/sdks/go/codegen_go/src/lib.rs (1)
22-24:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThe
unimplemented!()panic is still present.This issue was previously flagged: the hard panic in the public
to_source_codeAPI will abort callers at runtime. As suggested in the earlier review, replace it with a non-panicking stub that returnsHashMap::new()until the Go emitter is implemented.🤖 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/go/codegen_go/src/lib.rs` around lines 22 - 24, The public function to_source_code currently calls unimplemented!() which panics; replace that panic with a safe stub return (e.g., return an empty HashMap) so callers don't abort at runtime. Locate the to_source_code function in lib.rs and change its body to construct and return HashMap::new() (matching the function's return type) until the Go emitter implementation is added.baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs (1)
23-25:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThe
unimplemented!()panic is still breaking CI builds.This issue was previously flagged with extensive CI failure evidence. The panic in
to_source_codeis actively failingcargo test,cargo build, andcargo docacross Linux, macOS, and Windows platforms. Replace it with a non-panicking stub that returnsHashMap::new()as suggested in the earlier review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs` around lines 23 - 25, The to_source_code function currently calls unimplemented!() which panics and breaks CI; change its body to return an empty HashMap instead (e.g., return std::collections::HashMap::new()) so the Node.js emitter stub no longer panics; ensure the function signature of codegen_nodejs::to_source_code remains unchanged and import or fully qualify std::collections::HashMap if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/sdk_tests/crates/python_pydantic2/src/lib.rs`:
- Around line 1-8: Update the module docs at the top of lib.rs to reflect the
current generated test suite: change the phrase "four tests per fixture wrapping
`uv sync` and `uv run {ruff,pyright,pytest}`" to state that the generator emits
three tests per fixture (`ruff`, `pyright`, `pytest`) and remove any mention of
the `sync_only` CI pre-warm hook; keep the reference to the generator macro
sdk_test_build::python_pydantic2::test_suite! and the build output file
baml_language/sdk_tests/build/src/python_pydantic2.rs so readers can see where
the tests are generated.
---
Duplicate comments:
In @.github/workflows/cargo-tests.reusable.yaml:
- Around line 319-325: Add an explicit lib test step to the CI by inserting a
new GitHub Actions step (e.g., name it "Run lib tests") that runs `cargo test
--lib --all-features` with working-directory set to baml_language, and place it
before the existing "Run sdk tests" step (the step that runs `cargo nextest run
--all-features -E 'package(/^sdk_test_/)'`) so the workflow performs the direct
lib-test invocation required by the guidelines.
In `@baml_language/sdk_tests/README.md`:
- Around line 22-24: Update the README entry for sdk_test_nodejs to remove the
stale reference to the `sync_only` check and reflect the current Node scaffold:
mention only the two checks emitted (`tsc` via `pnpm exec tsc --noEmit` and
`jest` via `pnpm exec jest`) in the `sdk_test_nodejs` description so
contributors see the correct test selection; ensure the symbol `sdk_test_nodejs`
remains and `sync_only` is no longer listed.
- Around line 113-120: The "How It Works" section currently treats Python
behavior as universal; update the README's "How It Works" paragraph(s) to
explicitly separate Python and Node flows (or make behavior target-specific):
state that for Python fixtures overlays are symlinked into generated/<fixture>/
with fallback copy on Windows, pyproject.toml is written per-fixture, and
OUT_DIR/<generator>_tests.rs emits the four #[test] fns; and for the Node target
describe the differing overlay/copy semantics, any different generated manifest
(e.g., package.json) behavior, and the different test-generation or CI sync_only
pre-warm behavior referenced earlier (also update the similar text at the later
block around lines 134-139). Ensure the README mentions both pyproject.toml and
package.json where appropriate and clarifies which behavior applies to which
target.
In `@baml_language/sdks/go/codegen_go/src/lib.rs`:
- Around line 22-24: The public function to_source_code currently calls
unimplemented!() which panics; replace that panic with a safe stub return (e.g.,
return an empty HashMap) so callers don't abort at runtime. Locate the
to_source_code function in lib.rs and change its body to construct and return
HashMap::new() (matching the function's return type) until the Go emitter
implementation is added.
In `@baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs`:
- Around line 23-25: The to_source_code function currently calls
unimplemented!() which panics and breaks CI; change its body to return an empty
HashMap instead (e.g., return std::collections::HashMap::new()) so the Node.js
emitter stub no longer panics; ensure the function signature of
codegen_nodejs::to_source_code remains unchanged and import or fully qualify
std::collections::HashMap if necessary.
🪄 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: c2a24b8b-e97b-40c6-8b57-a98a7562f511
⛔ Files ignored due to path filters (14)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_inbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_outbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/go.sumis excluded by!**/*.sumbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/sdks/nodejs/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (127)
.github/workflows/cargo-tests.reusable.yaml.github/workflows/ci.yamlbaml_language/Cargo.tomlbaml_language/crates/bridge_ctypes/README.mdbaml_language/sdk_tests/README.mdbaml_language/sdk_tests/build/Cargo.tomlbaml_language/sdk_tests/build/src/lib.rsbaml_language/sdk_tests/build/src/nodejs.rsbaml_language/sdk_tests/build/src/python_pydantic2.rsbaml_language/sdk_tests/crates/docstrings_etc/Cargo.tomlbaml_language/sdk_tests/crates/docstrings_etc/build.rsbaml_language/sdk_tests/crates/docstrings_etc/src/lib.rsbaml_language/sdk_tests/crates/llm_functions/Cargo.tomlbaml_language/sdk_tests/crates/llm_functions/build.rsbaml_language/sdk_tests/crates/llm_functions/src/lib.rsbaml_language/sdk_tests/crates/nodejs/Cargo.tomlbaml_language/sdk_tests/crates/nodejs/build.rsbaml_language/sdk_tests/crates/nodejs/customizable/docstrings_etc/main.test.tsbaml_language/sdk_tests/crates/nodejs/customizable/llm_functions/main.test.tsbaml_language/sdk_tests/crates/nodejs/customizable/type_shapes/main.test.tsbaml_language/sdk_tests/crates/nodejs/src/lib.rsbaml_language/sdk_tests/crates/python_pydantic2/Cargo.tomlbaml_language/sdk_tests/crates/python_pydantic2/build.rsbaml_language/sdk_tests/crates/python_pydantic2/customizable/docstrings_etc/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/llm_functions/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/llm_functions/test_streaming_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_generic_method.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/src/lib.rsbaml_language/sdk_tests/crates/type_shapes/Cargo.tomlbaml_language/sdk_tests/crates/type_shapes/build.rsbaml_language/sdk_tests/crates/type_shapes/src/lib.rsbaml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_docs/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_ipsum/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_ipsum/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases_consumer/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_class_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_enums/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_forward_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/methods.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_ipsum/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lists/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_literals/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/cross_uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/streams.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_maps/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_media/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_optional/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_primitives/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_recursion/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/README.mdbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_a/ns_b/ns_c/ns_d/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_buzz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_unions/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_void/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/root.bamlbaml_language/sdks/go/bridge_go/build.shbaml_language/sdks/go/bridge_go/cffi/bridge.hbaml_language/sdks/go/bridge_go/cffi/exports.gobaml_language/sdks/go/bridge_go/cffi/lib.gobaml_language/sdks/go/bridge_go/cffi/types.gobaml_language/sdks/go/bridge_go/go.modbaml_language/sdks/go/bridge_go/pkg/callbacks.gobaml_language/sdks/go/bridge_go/pkg/errors.gobaml_language/sdks/go/bridge_go/pkg/handle.gobaml_language/sdks/go/bridge_go/pkg/init_callbacks.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/go/bridge_go/pkg/runtime.gobaml_language/sdks/go/bridge_go/pkg/tests/call_function_test.gobaml_language/sdks/go/bridge_go/pkg/tests/engine_test.gobaml_language/sdks/go/bridge_go/pkg/tests/handle_test.gobaml_language/sdks/go/bridge_go/pkg/types.gobaml_language/sdks/go/codegen_go/Cargo.tomlbaml_language/sdks/go/codegen_go/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/Cargo.tomlbaml_language/sdks/nodejs/bridge_nodejs/build.rsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.tsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.jsbaml_language/sdks/nodejs/bridge_nodejs/errors.d.tsbaml_language/sdks/nodejs/bridge_nodejs/errors.jsbaml_language/sdks/nodejs/bridge_nodejs/index.d.tsbaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/package.jsonbaml_language/sdks/nodejs/bridge_nodejs/protobaml_language/sdks/nodejs/bridge_nodejs/proto.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rsbaml_language/sdks/nodejs/bridge_nodejs/src/errors.rsbaml_language/sdks/nodejs/bridge_nodejs/src/handle.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/collector.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/host_span_manager.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/mod.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/run_jest.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tsconfig.jsonbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/generated-header.txtbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.jsbaml_language/sdks/nodejs/codegen_nodejs/Cargo.tomlbaml_language/sdks/nodejs/codegen_nodejs/src/lib.rsmise.tomlpnpm-workspace.yaml
💤 Files with no reviewable changes (38)
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_class_refs/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_foo/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_media/types.baml
- baml_language/sdk_tests/crates/docstrings_etc/Cargo.toml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_foo/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/uses.baml
- baml_language/sdk_tests/crates/llm_functions/Cargo.toml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_lorem/uses.baml
- baml_language/sdk_tests/crates/llm_functions/src/lib.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_a/ns_b/ns_c/ns_d/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_forward_refs/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/root.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_ipsum/uses.baml
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/cross_uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_recursion/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_optional/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_primitives/types.baml
- baml_language/sdk_tests/crates/type_shapes/Cargo.toml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_buzz/ns_foo/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_enums/types.baml
- baml_language/sdk_tests/crates/llm_functions/build.rs
- baml_language/sdk_tests/crates/docstrings_etc/build.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_maps/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/methods.baml
- baml_language/sdk_tests/crates/type_shapes/src/lib.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_unions/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_literals/types.baml
- baml_language/sdk_tests/crates/docstrings_etc/src/lib.rs
- baml_language/sdk_tests/crates/type_shapes/build.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lists/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/uses.baml
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/streams.baml
- baml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_generic_method.py
✅ Files skipped from review due to trivial changes (5)
- baml_language/sdks/nodejs/codegen_nodejs/Cargo.toml
- pnpm-workspace.yaml
- mise.toml
- baml_language/sdk_tests/crates/nodejs/Cargo.toml
- baml_language/sdk_tests/crates/python_pydantic2/Cargo.toml
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
baml_language/sdk_tests/README.md (2)
113-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake customizable staging behavior target-specific.
This step says overlays are symlinked with Windows fallback copy, but Node.js fixtures are copied on all platforms.
Suggested doc update
- - Symlinks each file in `customizable/<fixture>/` into - `generated/<fixture>/` (falls back to copy on Windows without - Developer Mode). + - Stages each file in `customizable/<fixture>/` into + `generated/<fixture>/` (Python uses symlinks with Windows fallback copy; + Node.js uses copy on all platforms).🤖 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 113 - 115, Update the README so the customizable staging behavior is target-specific: clarify that overlay files in customizable/<fixture>/ are symlinked into generated/<fixture>/ with a Windows fallback to copy, but Node.js fixtures are always copied on all platforms (no symlink attempted); update the sentences describing the staging step to explicitly mention "overlay files" vs "Node.js fixtures" and the platform-specific symlink fallback behavior.
20-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
sync_onlydocumentation is stale and no longer matches the generated tests/CI flow.The current scaffolds and workflow no longer expose a
sync_onlytest, so these check counts, command examples, and CI pre-warm steps are inaccurate.Suggested doc update
-- **`sdk_test_python_pydantic2`** — Python + pydantic2. Four checks - per fixture: `sync_only` (`uv sync`), `ruff`, `pyright`, `pytest`. +-- **`sdk_test_python_pydantic2`** — Python + pydantic2. Three checks + per fixture: `ruff`, `pyright`, `pytest`. -- **`sdk_test_nodejs`** — TypeScript on Node.js. Three checks per - fixture: `sync_only` (`pnpm install --ignore-workspace`), `tsc` +-- **`sdk_test_nodejs`** — TypeScript on Node.js. Two checks per + fixture: `tsc` (`pnpm exec tsc --noEmit`), `jest` (`pnpm exec jest`). ... -# Pre-warm the toolchain cache (CI does this before parallel fan-out) -cargo test -p sdk_test_python_pydantic2 sync_only -- --test-threads=1 +# Build-time setup happens in build.rs; tests run toolchain checks directly +cargo test -p sdk_test_python_pydantic2 type_shapes::pyright ... - - Emits `OUT_DIR/<generator>_tests.rs` — ... with the four `#[test]` fns ... + - Emits `OUT_DIR/<generator>_tests.rs` — ... with one `#[test]` per configured check ... ... -3. The four `#[test]` fns per fixture all call ... +3. The generated `#[test]` fns per fixture call ... -4. **CI pre-warm**: ... `nextest run -E 'test(=sync_only)' --test-threads=1` ... +4. **CI execution**: sdk tests run through `nextest` on `sdk_test_*` crates; setup is done in build.rs.Also applies to: 38-40, 119-120, 128-139
🤖 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 20 - 24, Update the README to remove or correct stale references to the removed "sync_only" test flow: for each fixture name such as sdk_test_python_pydantic2 and sdk_test_nodejs, adjust the described check counts and example commands to match the current generated tests/CI (remove "sync_only" from the list, update command examples like `uv sync`/`pnpm install --ignore-workspace`, and any CI pre-warm steps), and apply the same fixes to the other affected sections (lines referenced around 38-40, 119-120, 128-139) so the documented checks, commands, and counts reflect the actual tests run today..github/workflows/cargo-tests.reusable.yaml (1)
323-325:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an explicit
cargo test --libstep in the sdk-tests flow.Line 324 currently runs nextest directly after
cargo test --no-run; this misses the required direct lib-test invocation for Rust changes.Suggested patch
- name: "Build tests with --timings" run: cargo test --no-run --all-features --timings working-directory: baml_language + - name: "Run lib tests" + run: cargo test --lib --all-features + working-directory: baml_language + - name: "Run sdk tests" run: cargo nextest run --all-features -E 'package(/^sdk_test_/)' working-directory: baml_languageAs per coding guidelines: "Always run
cargo test --libif you changed any Rust 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 @.github/workflows/cargo-tests.reusable.yaml around lines 323 - 325, The "Run sdk tests" step currently executes "cargo nextest run ..." in the baml_language working-directory but omits the required direct library test; add an explicit step that runs "cargo test --lib" (in the same working-directory baml_language) before the "cargo nextest run ..." invocation so that library tests are executed for Rust changes; ensure the new step is placed after any existing "cargo test --no-run" step and before the nextest execution to satisfy the guideline to always run cargo test --lib when Rust code changed.baml_language/sdks/go/codegen_go/src/lib.rs (1)
22-24:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
unimplemented!will panic callers at runtime.The
unimplemented!()macro causes a hard panic whento_source_codeis invoked. Since this is a public API, prefer returning an emptyHashMapas a non-panicking placeholder until the Go emitter is implemented.🤖 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/go/codegen_go/src/lib.rs` around lines 22 - 24, The public function to_source_code in module codegen_go currently calls unimplemented!(), which panics; replace that panic with returning an empty std::collections::HashMap to provide a non-panicking placeholder until the Go emitter is implemented. Remove the unimplemented!() call, construct and return HashMap::new() (or the equivalent empty map type expected by to_source_code's signature), and ensure any necessary use/import for std::collections::HashMap is present so the function compiles without changing its public API.baml_language/sdk_tests/crates/python_pydantic2/src/lib.rs (1)
1-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winModule docs still reference outdated test count and removed
sync_onlyhook.Lines 1–8 still say "four tests per fixture wrapping
uv syncanduv run {ruff,pyright,pytest}" and mention async_onlyCI pre-warm hook, but this was already flagged and marked as addressed in commit b335e0d. The current code still contains the stale documentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/crates/python_pydantic2/src/lib.rs` around lines 1 - 8, The module doc in lib.rs is stale: it still claims "four tests per fixture wrapping `uv sync` and `uv run {ruff,pyright,pytest}`" and mentions a removed `sync_only` CI hook; update the top-level documentation comment to match the current test-suite generation (via sdk_test_build::python_pydantic2::test_suite!) and remove any reference to the `sync_only` hook and the old four-test count, e.g., replace the sentence describing the test count and `sync_only` with a short, accurate summary of what the macro generates and where to see the expansion (keep references to sdk_test_build::python_pydantic2::test_suite! and sdk_tests/build/src/python_pydantic2.rs for details).baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs (1)
23-25:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
unimplemented!is actively breaking CI builds.The past review comment documents multiple CI failures across all platforms (Linux, macOS, Windows) caused by this panic during
cargo buildandcargo testruns. Thesdk_test_nodejsbuild script is invoking this stub, causing build failures. Replace with a non-panicking placeholder (e.g.,HashMap::new()) to unblock CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs` around lines 23 - 25, The unimplemented! macro inside the Node.js emitter stub (codegen_nodejs::to_source_code in lib.rs) causes panics during cargo build/tests; replace the panic with a safe, non-panicking placeholder return (for example return an empty HashMap via HashMap::new()) so the function no longer panics and CI can proceed; update the function body to construct and return the empty map instead of calling unimplemented! and ensure any required use/imports for HashMap are present.
🤖 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.
Duplicate comments:
In @.github/workflows/cargo-tests.reusable.yaml:
- Around line 323-325: The "Run sdk tests" step currently executes "cargo
nextest run ..." in the baml_language working-directory but omits the required
direct library test; add an explicit step that runs "cargo test --lib" (in the
same working-directory baml_language) before the "cargo nextest run ..."
invocation so that library tests are executed for Rust changes; ensure the new
step is placed after any existing "cargo test --no-run" step and before the
nextest execution to satisfy the guideline to always run cargo test --lib when
Rust code changed.
In `@baml_language/sdk_tests/crates/python_pydantic2/src/lib.rs`:
- Around line 1-8: The module doc in lib.rs is stale: it still claims "four
tests per fixture wrapping `uv sync` and `uv run {ruff,pyright,pytest}`" and
mentions a removed `sync_only` CI hook; update the top-level documentation
comment to match the current test-suite generation (via
sdk_test_build::python_pydantic2::test_suite!) and remove any reference to the
`sync_only` hook and the old four-test count, e.g., replace the sentence
describing the test count and `sync_only` with a short, accurate summary of what
the macro generates and where to see the expansion (keep references to
sdk_test_build::python_pydantic2::test_suite! and
sdk_tests/build/src/python_pydantic2.rs for details).
In `@baml_language/sdk_tests/README.md`:
- Around line 113-115: Update the README so the customizable staging behavior is
target-specific: clarify that overlay files in customizable/<fixture>/ are
symlinked into generated/<fixture>/ with a Windows fallback to copy, but Node.js
fixtures are always copied on all platforms (no symlink attempted); update the
sentences describing the staging step to explicitly mention "overlay files" vs
"Node.js fixtures" and the platform-specific symlink fallback behavior.
- Around line 20-24: Update the README to remove or correct stale references to
the removed "sync_only" test flow: for each fixture name such as
sdk_test_python_pydantic2 and sdk_test_nodejs, adjust the described check counts
and example commands to match the current generated tests/CI (remove "sync_only"
from the list, update command examples like `uv sync`/`pnpm install
--ignore-workspace`, and any CI pre-warm steps), and apply the same fixes to the
other affected sections (lines referenced around 38-40, 119-120, 128-139) so the
documented checks, commands, and counts reflect the actual tests run today.
In `@baml_language/sdks/go/codegen_go/src/lib.rs`:
- Around line 22-24: The public function to_source_code in module codegen_go
currently calls unimplemented!(), which panics; replace that panic with
returning an empty std::collections::HashMap to provide a non-panicking
placeholder until the Go emitter is implemented. Remove the unimplemented!()
call, construct and return HashMap::new() (or the equivalent empty map type
expected by to_source_code's signature), and ensure any necessary use/import for
std::collections::HashMap is present so the function compiles without changing
its public API.
In `@baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs`:
- Around line 23-25: The unimplemented! macro inside the Node.js emitter stub
(codegen_nodejs::to_source_code in lib.rs) causes panics during cargo
build/tests; replace the panic with a safe, non-panicking placeholder return
(for example return an empty HashMap via HashMap::new()) so the function no
longer panics and CI can proceed; update the function body to construct and
return the empty map instead of calling unimplemented! and ensure any required
use/imports for HashMap are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 85f0a00f-7820-4838-bad5-9423e0bb3ee7
⛔ Files ignored due to path filters (14)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_inbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_outbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/go/bridge_go/go.sumis excluded by!**/*.sumbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/sdks/nodejs/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (127)
.github/workflows/cargo-tests.reusable.yaml.github/workflows/ci.yamlbaml_language/Cargo.tomlbaml_language/crates/bridge_ctypes/README.mdbaml_language/sdk_tests/README.mdbaml_language/sdk_tests/build/Cargo.tomlbaml_language/sdk_tests/build/src/lib.rsbaml_language/sdk_tests/build/src/nodejs.rsbaml_language/sdk_tests/build/src/python_pydantic2.rsbaml_language/sdk_tests/crates/docstrings_etc/Cargo.tomlbaml_language/sdk_tests/crates/docstrings_etc/build.rsbaml_language/sdk_tests/crates/docstrings_etc/src/lib.rsbaml_language/sdk_tests/crates/llm_functions/Cargo.tomlbaml_language/sdk_tests/crates/llm_functions/build.rsbaml_language/sdk_tests/crates/llm_functions/src/lib.rsbaml_language/sdk_tests/crates/nodejs/Cargo.tomlbaml_language/sdk_tests/crates/nodejs/build.rsbaml_language/sdk_tests/crates/nodejs/customizable/docstrings_etc/main.test.tsbaml_language/sdk_tests/crates/nodejs/customizable/llm_functions/main.test.tsbaml_language/sdk_tests/crates/nodejs/customizable/type_shapes/main.test.tsbaml_language/sdk_tests/crates/nodejs/src/lib.rsbaml_language/sdk_tests/crates/python_pydantic2/Cargo.tomlbaml_language/sdk_tests/crates/python_pydantic2/build.rsbaml_language/sdk_tests/crates/python_pydantic2/customizable/docstrings_etc/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/llm_functions/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/llm_functions/test_streaming_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_generic_method.pybaml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/src/lib.rsbaml_language/sdk_tests/crates/type_shapes/Cargo.tomlbaml_language/sdk_tests/crates/type_shapes/build.rsbaml_language/sdk_tests/crates/type_shapes/src/lib.rsbaml_language/sdk_tests/fixtures/docstrings_etc/baml_src/ns_docs/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_ipsum/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_ipsum/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases_consumer/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_class_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_enums/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_forward_refs/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/methods.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_ipsum/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lists/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_literals/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/cross_uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/streams.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_maps/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_media/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_optional/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_primitives/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_recursion/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/README.mdbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_a/ns_b/ns_c/ns_d/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_buzz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_foo/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_lorem/uses.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_unions/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_void/types.bamlbaml_language/sdk_tests/fixtures/type_shapes/baml_src/root.bamlbaml_language/sdks/go/bridge_go/build.shbaml_language/sdks/go/bridge_go/cffi/bridge.hbaml_language/sdks/go/bridge_go/cffi/exports.gobaml_language/sdks/go/bridge_go/cffi/lib.gobaml_language/sdks/go/bridge_go/cffi/types.gobaml_language/sdks/go/bridge_go/go.modbaml_language/sdks/go/bridge_go/pkg/callbacks.gobaml_language/sdks/go/bridge_go/pkg/errors.gobaml_language/sdks/go/bridge_go/pkg/handle.gobaml_language/sdks/go/bridge_go/pkg/init_callbacks.gobaml_language/sdks/go/bridge_go/pkg/proto.gobaml_language/sdks/go/bridge_go/pkg/runtime.gobaml_language/sdks/go/bridge_go/pkg/tests/call_function_test.gobaml_language/sdks/go/bridge_go/pkg/tests/engine_test.gobaml_language/sdks/go/bridge_go/pkg/tests/handle_test.gobaml_language/sdks/go/bridge_go/pkg/types.gobaml_language/sdks/go/codegen_go/Cargo.tomlbaml_language/sdks/go/codegen_go/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/Cargo.tomlbaml_language/sdks/nodejs/bridge_nodejs/build.rsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.tsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.jsbaml_language/sdks/nodejs/bridge_nodejs/errors.d.tsbaml_language/sdks/nodejs/bridge_nodejs/errors.jsbaml_language/sdks/nodejs/bridge_nodejs/index.d.tsbaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/package.jsonbaml_language/sdks/nodejs/bridge_nodejs/protobaml_language/sdks/nodejs/bridge_nodejs/proto.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rsbaml_language/sdks/nodejs/bridge_nodejs/src/errors.rsbaml_language/sdks/nodejs/bridge_nodejs/src/handle.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/collector.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/host_span_manager.rsbaml_language/sdks/nodejs/bridge_nodejs/src/types/mod.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/run_jest.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tsconfig.jsonbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/generated-header.txtbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.jsbaml_language/sdks/nodejs/codegen_nodejs/Cargo.tomlbaml_language/sdks/nodejs/codegen_nodejs/src/lib.rsmise.tomlpnpm-workspace.yaml
💤 Files with no reviewable changes (38)
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_foo/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/cross_uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_ipsum/uses.baml
- baml_language/sdk_tests/crates/docstrings_etc/Cargo.toml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_enums/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/root.baml
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.baml
- baml_language/sdk_tests/crates/llm_functions/src/lib.rs
- baml_language/sdk_tests/crates/type_shapes/src/lib.rs
- baml_language/sdk_tests/crates/type_shapes/Cargo.toml
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_foo/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_a/ns_b/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_primitives/types.baml
- baml_language/sdk_tests/crates/llm_functions/Cargo.toml
- baml_language/sdk_tests/crates/type_shapes/build.rs
- baml_language/sdk_tests/crates/llm_functions/build.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_unions/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_fizz/ns_buzz/ns_foo/types.baml
- baml_language/sdk_tests/crates/docstrings_etc/src/lib.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_class_refs/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_media/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_aliases/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_forward_refs/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lists/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_lorem/uses.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_literals/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_optional/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_recursion/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_maps/types.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_symbol_collisions/ns_a/ns_b/ns_c/ns_d/uses.baml
- baml_language/sdk_tests/crates/docstrings_etc/build.rs
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/streams.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_generics/methods.baml
- baml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_lorem/uses.baml
- baml_language/sdk_tests/crates/python_pydantic2/customizable/type_shapes/test_generic_method.py
✅ Files skipped from review due to trivial changes (4)
- pnpm-workspace.yaml
- baml_language/sdks/nodejs/codegen_nodejs/Cargo.toml
- baml_language/crates/bridge_ctypes/README.md
- mise.toml
sxlijin
changed the title
Three combined restructures:
1. **Split sdk_tests/build into two crates**:
- `harness_setup` (`[build-dependencies]`) — codegen, uv sync /
pnpm install, OUT_DIR scaffold emission, BuildDiagnostics.
Keeps the heavy codegen_* / baml_project deps.
- `harness_runner` (`[dev-dependencies]`) — run_test_cmd, the
shared `build_diagnostics!` macro, and one
`<generator>::test_suite!()` macro per generator. Std-only
deps. The scaffold emitted by harness_setup is now a pure
sequence of `::harness_runner::*` macro / function
invocations; no test bodies are authored at build time.
2. **Rename nodejs → nodejs_typescript**:
- `sdk_test_nodejs` → `sdk_test_nodejs_typescript`
- `sdk_tests/crates/nodejs/` → `crates/nodejs_typescript/`
- `harness_setup/src/nodejs.rs` → `nodejs_typescript.rs`
3. **Restructure to fixture-first under each crate**:
- `sdk_tests/fixtures/<F>/` holds only `baml_src/` (generator-
agnostic input).
- Per-generator test code lives under
`crates/<G>/<F>/customizable/` (tracked) and
`crates/<G>/<F>/generated/` (gitignored) — one subtree per
fixture so `rm -r` removes a fixture cleanly.
`harness_runner::build_diagnostics!` takes an optional
`(ignore = "reason")` form; nodejs_typescript's scaffold uses it
while `codegen_nodejs` is a stub.
cargo test -p sdk_test_python_pydantic2: 10 passed.
cargo test -p sdk_test_nodejs_typescript: 7 ignored, 0 failed.
cargo doc --no-deps -D warnings: clean.
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores
Documentation