Repository navigation
chore: rename sdk generation crates from codegen_* to sdkgen_* - #3657
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR renames SDK code-generator crates from ChangesSDK Generator Crate Renaming and Naming Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
![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: 3f95c6c9c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+540
to
+550
| params.push(Param { | ||
| name: Name::new("client"), | ||
| type_expr: Some(SpannedTypeExpr { | ||
| expr: TypeExpr::Path { | ||
| segments: vec![Name::new("baml"), Name::new("llm"), Name::new("Client")], | ||
| generic_args: vec![], | ||
| attrs: vec![], | ||
| }, | ||
| span, | ||
| }), | ||
| default: Some(crate::ast::DefaultExprId::new(default_expr)), |
There was a problem hiding this comment.
For every LLM function with a client declaration, this appends a defaulted client parameter that flows into codegen. I checked baml_language/sdks/nodejs/sdkgen_typescript_node/src/leaf.rs around render_function_ts: the TypeScript emitter builds the cast signature from all f.param_names/arg_tys and does not inspect FunctionArgumentDefault, so generated TS signatures become e.g. (input: string, client: baml.llm.Client) => ... and callers of the normal default-client form Ask("hi") get a type error. Python has a positional-count/default handling path, but the TS SDK currently does not, so this injection should either be omitted from TS signatures or rendered as optional there.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/baml_compiler2_ast/src/lower_cst.rs (1)
275-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReserve
clientbefore lowering defaults.Line 275 lowers defaults against the original signature, then Line 317 deletes/reinserts
client. That means a case likefunction F(client: string, alias: string = client)will rebindalias's default to the synthesizedbaml.llm.Clientparam instead of the rejected user param, and defaults/env refs from the removed param still survive indefaults. Please filter the reserved LLMclientparameter out before callinglower_params_with_defaults, or re-lower defaults after rewriting the parameter list. A regression test for a surviving default that referencesclientwould catch this.Also applies to: 317-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_ast/src/lower_cst.rs` around lines 275 - 287, The lowering currently calls lower_params_with_defaults on the original param_list() before reserving/removing the synthesized LLM "client" parameter, which lets defaults (and env var refs) bind to the user "client" param that will later be deleted; modify the code so you filter out the reserved/synthesized "client" parameter from the param_list() (or otherwise remove it from the iterator) before calling lower_params_with_defaults (the call site using name.as_str(), ¶meter_context, diags, true, env_var_refs), ensuring FunctionDefaults returned do not contain references to the removed param; alternatively, perform the reservation/rewrite of the client param first and then re-run lower_params_with_defaults against the rewritten parameter list so defaults are lowered against the final signature.
🧹 Nitpick comments (1)
baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/lib.rs (1)
1921-1988: ⚡ Quick winConsider verifying async variants in .pyi for completeness.
The test thoroughly verifies that the
clientparameter renders correctly in both function and companion signatures. However, it only checks the sync variants in the.pyifile (lines 1983-1987). Following the pattern established by similar parameter tests (e.g.,function_defaults_render_keyword_only_signature_and_positional_limitat lines 1914-1918), consider adding assertions for the async variants to ensure complete coverage:assert!(pyi.contains( "async def extract_resume_async(resume: str, *, client: baml.llm.Client = ...) -> str: ...\n" )); assert!(pyi.contains( "async def extract_resume__build_request_async(resume: str, *, client: baml.llm.Client = ...) -> baml.http.Request: ...\n" ));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/lib.rs` around lines 1921 - 1988, The test llm_default_client_argument_renders_in_function_and_companion_signatures only asserts the sync .pyi signatures for extract_resume and extract_resume$build_request; add assertions for the async variants to mirror coverage. Inside that test (referencing the symbols extract_resume and extract_resume$build_request and the pyi variable), add checks asserting pyi.contains for "async def extract_resume_async(resume: str, *, client: baml.llm.Client = ...) -> str: ..." and for "async def extract_resume__build_request_async(resume: str, *, client: baml.llm.Client = ...) -> baml.http.Request: ...".
🤖 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.
Outside diff comments:
In `@baml_language/crates/baml_compiler2_ast/src/lower_cst.rs`:
- Around line 275-287: The lowering currently calls lower_params_with_defaults
on the original param_list() before reserving/removing the synthesized LLM
"client" parameter, which lets defaults (and env var refs) bind to the user
"client" param that will later be deleted; modify the code so you filter out the
reserved/synthesized "client" parameter from the param_list() (or otherwise
remove it from the iterator) before calling lower_params_with_defaults (the call
site using name.as_str(), ¶meter_context, diags, true, env_var_refs),
ensuring FunctionDefaults returned do not contain references to the removed
param; alternatively, perform the reservation/rewrite of the client param first
and then re-run lower_params_with_defaults against the rewritten parameter list
so defaults are lowered against the final signature.
---
Nitpick comments:
In `@baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/lib.rs`:
- Around line 1921-1988: The test
llm_default_client_argument_renders_in_function_and_companion_signatures only
asserts the sync .pyi signatures for extract_resume and
extract_resume$build_request; add assertions for the async variants to mirror
coverage. Inside that test (referencing the symbols extract_resume and
extract_resume$build_request and the pyi variable), add checks asserting
pyi.contains for "async def extract_resume_async(resume: str, *, client:
baml.llm.Client = ...) -> str: ..." and for "async def
extract_resume__build_request_async(resume: str, *, client: baml.llm.Client =
...) -> baml.http.Request: ...".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bbafee14-043c-4d36-8d95-7a42f360c599
⛔ Files ignored due to path filters (44)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/architecture/architecture.svgis excluded by!**/*.svgbaml_language/crates/baml_tests/snapshots/baml_src/type_reflection.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/comment_in_type/baml_tests__compiles__comment_in_type__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/comment_in_type/baml_tests__compiles__comment_in_type__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/comment_in_type/baml_tests__compiles__comment_in_type__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/comment_in_type/baml_tests__compiles__comment_in_type__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/json_llm_return_type/baml_tests__compiles__json_llm_return_type__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/json_llm_return_type/baml_tests__compiles__json_llm_return_type__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/json_llm_return_type/baml_tests__compiles__json_llm_return_type__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/json_llm_return_type/baml_tests__compiles__json_llm_return_type__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/llm_image_outputs/baml_tests__compiles__llm_image_outputs__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/llm_image_outputs/baml_tests__compiles__llm_image_outputs__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/llm_image_outputs/baml_tests__compiles__llm_image_outputs__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/llm_image_outputs/baml_tests__compiles__llm_image_outputs__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/stream_llm_inferred_typeargs/baml_tests__compiles__stream_llm_inferred_typeargs__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/stream_llm_inferred_typeargs/baml_tests__compiles__stream_llm_inferred_typeargs__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/stream_llm_inferred_typeargs/baml_tests__compiles__stream_llm_inferred_typeargs__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/stream_llm_inferred_typeargs/baml_tests__compiles__stream_llm_inferred_typeargs__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/type_builder_errors/baml_tests__compiles__type_builder_errors__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/type_builder_errors/baml_tests__compiles__type_builder_errors__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/type_builder_errors/baml_tests__compiles__type_builder_errors__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/type_builder_errors/baml_tests__compiles__type_builder_errors__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/type_builder_test/baml_tests__compiles__type_builder_test__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/type_builder_test/baml_tests__compiles__type_builder_test__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/type_builder_test/baml_tests__compiles__type_builder_test__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/type_builder_test/baml_tests__compiles__type_builder_test__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/basic_types/baml_tests__diagnostic_errors__basic_types__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/basic_types/baml_tests__diagnostic_errors__basic_types__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/control_flow/baml_tests__diagnostic_errors__control_flow__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/control_flow/baml_tests__diagnostic_errors__control_flow__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/format_checks/baml_tests__diagnostic_errors__format_checks__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/format_checks/baml_tests__diagnostic_errors__format_checks__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/simple_function/baml_tests__diagnostic_errors__simple_function__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/simple_function/baml_tests__diagnostic_errors__simple_function__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__stream_expansion__stream_companion_preserves_generic_args_for_llm_return_type.snapis excluded by!**/*.snap
📒 Files selected for processing (48)
baml_language/Cargo.tomlbaml_language/crates/baml_cli/Cargo.tomlbaml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_compiler2_ast/src/companions.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lowering_diagnostic.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/functions_v2/duplicate_names.bamlbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_tests/src/compiler2_hir.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/sdk_tests/README.mdbaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_raises.pybaml_language/sdk_tests/crates/typescript_node/build.rsbaml_language/sdk_tests/harness_runner/Cargo.tomlbaml_language/sdk_tests/harness_runner/src/lib.rsbaml_language/sdk_tests/harness_setup/Cargo.tomlbaml_language/sdk_tests/harness_setup/src/python_pydantic2.rsbaml_language/sdk_tests/harness_setup/src/typescript_node.rsbaml_language/sdks/go/codegen_go/src/lib.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/Cargo.tomlbaml_language/sdks/nodejs/sdkgen_typescript_node/src/emit/class.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/emit/enum_.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/emit/function.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/emit/method.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/emit/mod.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/emit/type_alias.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/emit/typemap_file.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/leaf.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/lib.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/routing.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/translate_ty.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/Cargo.tomlbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/emit/class.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/emit/enum_.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/emit/function.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/emit/method.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/emit/mod.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/emit/type_alias.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/emit/typemap_file.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/leaf.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/lib.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/routing.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/translate_ty.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/utils.rsbaml_language/sdks/python/src/baml_core/typemap.py
💤 Files with no reviewable changes (1)
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/functions_v2/duplicate_names.baml
Binary size checks passed✅ 7 passed
Generated by |
sxlijin
force-pushed
the
push-qyksnqurnxoq
branch
from
June 3, 2026 07:27
3f95c6c to
2e96240
Compare
sxlijin
force-pushed
the
push-qyksnqurnxoq
branch
from
June 3, 2026 07:29
2e96240 to
0acf14e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/tools_stow/src/main.rs (1)
13-17: 💤 Low valueOptional: Consider documenting the
approved_suffixesfeature in the top-level rules comment.The top-level doc comment describes the naming rules but doesn't mention the new
approved_suffixesconfiguration. While the field-level documentation (lines 70-73) is clear, adding a brief mention here would improve discoverability for users reading the rules overview.Example addition after line 17:
//! - Auto-allowed suffixes: `_types`, `_test`, `_tests` (no prefix needed) +//! - Per-namespace `approved_suffixes` accept entire tails verbatim (e.g., "python_pydantic2")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/tools_stow/src/main.rs` around lines 13 - 17, Add a brief mention of the approved_suffixes configuration to the top-level rules doc comment so readers see it in the overview: update the header comment block (the file-level doc comments at the top of main.rs) to include a short line like "Optional: `approved_suffixes` can be configured to allow additional crate name suffixes" and a one-sentence note about its purpose and that this field is documented in the `approved_suffixes` field-level docs; reference the `approved_suffixes` identifier so users can quickly find the detailed docs 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.
Nitpick comments:
In `@baml_language/crates/tools_stow/src/main.rs`:
- Around line 13-17: Add a brief mention of the approved_suffixes configuration
to the top-level rules doc comment so readers see it in the overview: update the
header comment block (the file-level doc comments at the top of main.rs) to
include a short line like "Optional: `approved_suffixes` can be configured to
allow additional crate name suffixes" and a one-sentence note about its purpose
and that this field is documented in the `approved_suffixes` field-level docs;
reference the `approved_suffixes` identifier so users can quickly find the
detailed docs later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 89a26ee9-62d8-43bd-9f1a-8c7b4aaa0015
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
baml_language/Cargo.tomlbaml_language/crates/tools_stow/src/main.rsbaml_language/sdks/go/sdkgen_go/Cargo.tomlbaml_language/sdks/go/sdkgen_go/src/lib.rsbaml_language/stow.toml
✅ Files skipped from review due to trivial changes (1)
- baml_language/sdks/go/sdkgen_go/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/stow.toml
9 of 10 tasks
5 tasks done
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
Refactor
Bug Fixes