bridge-cpp: C ABI header + baml-cpp tarball packaging (slice 0) - #4004
Merged
Conversation
First slice of the C++ bridge: make the bridge_cffi C ABI consumable as a
classic lib/ + include/ artifact.
- Commit cbindgen-generated include/baml_cffi.h as the reviewed ABI contract,
with a freshness test (BLESS=1 to regenerate). Config gains cpp_compat,
an include guard, enum variant prefixing, and parses sys_native /
bex_resource_types so HostDispatchFn/HostReleaseFn typedefs are emitted.
- scripts/package_cpp_tarball.sh (mise run package-cpp): builds the cdylib
and assembles baml-cpp-<version>-<target>/{lib,include,VERSION,LICENSE}
plus sha256. Picks up header-only bridge_cpp headers once they exist.
- scripts/smoke_cpp_tarball.sh (mise run smoke-cpp): compiles and runs a
C++17 program against the extracted tarball only and asserts version()
round-trips through the C ABI.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR adds C++ SDK generation, a C ABI header, protobuf-lite bindings, C++ tests and packaging tasks, manifest support for C++ artifacts, and cross-platform release verification. ChangesC++ SDK release
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant VerifyCppWorkflow
participant PlatformsJson
participant BridgeArtifact
participant RuntimeSmoke
ReleaseWorkflow->>VerifyCppWorkflow: pass release plan and source SHA
VerifyCppWorkflow->>PlatformsJson: derive supported C++ targets
VerifyCppWorkflow->>BridgeArtifact: download and verify bridge artifact
VerifyCppWorkflow->>RuntimeSmoke: compile and execute smoke test
RuntimeSmoke-->>VerifyCppWorkflow: return expected version
VerifyCppWorkflow-->>ReleaseWorkflow: complete verification
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Binary size checks passed✅ 7 passed
Generated by |
![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: 1
🤖 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/scripts/package_cpp_tarball.sh`:
- Around line 39-41: Update the Windows artifact selection in
package_cpp_tarball.sh to choose bridge_cffi.dll.lib for MSVC targets and
libbridge_cffi.dll.a for *-pc-windows-gnu targets. In smoke_cpp_tarball.sh, add
a Windows-compatible compile/run path without Unix rpath flags, or explicitly
skip Windows targets; apply the required changes at
baml_language/scripts/package_cpp_tarball.sh:39-41 and
baml_language/scripts/smoke_cpp_tarball.sh:45-46.
🪄 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: c4dacba2-8ee0-494c-aff7-930645c332ec
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
baml_language/Cargo.tomlbaml_language/crates/bridge_cffi/Cargo.tomlbaml_language/crates/bridge_cffi/cbindgen.tomlbaml_language/crates/bridge_cffi/include/baml_cffi.hbaml_language/crates/bridge_cffi/tests/header_is_current.rsbaml_language/mise.tomlbaml_language/scripts/package_cpp_tarball.shbaml_language/scripts/smoke_cpp_tarball.sh
…rofile) [profile.release] sets panic=abort, which would turn any engine panic into a SIGABRT of the host process. The cdylib is loaded into user processes and bridge_cffi's catch_unwind boundary needs unwinding to deliver BamlPanic envelopes, same as release-bridge-python / release-bridge-node.
- build2-cpp-sdk.reusable.yaml: 8-target matrix (native runners, musl-tools for musl, MSVC cross for aarch64 windows) that stamps the frozen release plan, runs package_cpp_tarball.sh, smoke-tests the tarball on unix targets, and uploads cpp-sdk-<target> artifacts. - release-baml-language.yml: build-cpp-sdk job wired like the python/node SDK builds (frozen plan + source SHA), gated in all-builds; tarballs attach to the baml-language-<version> GitHub release; manifest jobs (production and dry-run) collect them via --cpp-dir. - baml-release-manifests: --cpp-dir collects baml-cpp-<version>-<target> tarballs into a baml_cpp manifest key; artifact URLs point at the baml-language release tag the files are actually uploaded to. - baml_release: optional ToolchainManifest.baml_cpp field (schema stays 1, older wrappers ignore it) validated against the exact release target set, with round-trip tests.
Per bridge-release-contract.md: - release/platforms.json: machine-readable platform contract (target, runner, libc, smoke container, MSVC arch, per-SDK status). The cpp build workflow derives its matrix from it instead of copying the 8-target list; baml-release-manifests reads its TARGETS from it; a baml_release test pins SUPPORTED_RELEASE_TARGETS to it. - build2-cpp-sdk.reusable.yaml: inputs are required (no recomputed plan); build jobs only build/package/upload; a verify job downloads the assembled artifact per target and consumer-smokes it natively on all 8 - Alpine containers for musl (a glibc host is not a valid musl consumer), MSVC dev-shell smoke on Windows x64 and arm64 (arm64 build moves to the native windows-11-arm runner), checksum verification, and a version assertion against the frozen plan's canonical_version. - smoke_cpp_tarball.ps1: Windows consumer smoke via vswhere + VsDevShell. - smoke_cpp_tarball.sh: BAML_EXPECTED_VERSION assertion; tarball argument resolved relative to the caller before entering the workspace.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/build2-cpp-sdk.reusable.yaml (1)
74-84: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMatrix values spliced directly into script bodies (zizmor template-injection).
matrix.target(Line 80),matrix.smoke_container(Line 150), andmatrix.msvc_arch(Line 158) are expanded directly intorun:/pwshscript text via${{ }}, rather than passed throughenv:. Today these values originate from the repo-committedrelease/platforms.json, so exploitability is low, but this is exactly the anti-pattern static analysis flags as a potential script-injection vector, and hardening it is cheap.♻️ Example hardening for line 80 (same pattern applies to 150, 158)
- name: Setup musl toolchain if: matrix.libc == 'musl' + env: + TARGET: ${{ matrix.target }} run: | set -euo pipefail sudo apt-get update sudo apt-get install -y musl-tools - target='${{ matrix.target }}' + target="$TARGET" cc_var="CC_$(echo "$target" | tr '-' '_')"Also applies to: 145-151, 153-158
🤖 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/build2-cpp-sdk.reusable.yaml around lines 74 - 84, Harden the workflow’s matrix-value handling by passing matrix.target, matrix.smoke_container, and matrix.msvc_arch through step-level env variables instead of interpolating them directly inside run or pwsh script bodies. Update the affected setup and smoke/container steps to reference those environment variables while preserving the existing command behavior.Source: Linters/SAST tools
🤖 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/scripts/smoke_cpp_tarball.sh`:
- Around line 19-20: Replace the ls-based assignment in the TARBALL selection
block with a guarded Bash glob that collects matching
target/cpp-dist/baml-cpp-*.tar.gz files and selects the newest candidate without
triggering set -e or pipefail. Preserve the existing empty-TARBALL error
handling so it runs when no archive matches.
---
Nitpick comments:
In @.github/workflows/build2-cpp-sdk.reusable.yaml:
- Around line 74-84: Harden the workflow’s matrix-value handling by passing
matrix.target, matrix.smoke_container, and matrix.msvc_arch through step-level
env variables instead of interpolating them directly inside run or pwsh script
bodies. Update the affected setup and smoke/container steps to reference those
environment variables while preserving the existing command behavior.
🪄 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: ea266aac-69bf-4c16-a966-2238c13746ca
📒 Files selected for processing (6)
.github/workflows/build2-cpp-sdk.reusable.yamlbaml_language/crates/baml_release/src/manifest.rsbaml_language/scripts/smoke_cpp_tarball.ps1baml_language/scripts/smoke_cpp_tarball.shrelease/platforms.jsonscripts/baml-release-manifests
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/crates/baml_release/src/manifest.rs
…ry, Future, runtime init) First bridge_cpp slice per the locked codegen spec, C++17 header-only at sdks/cpp/bridge_cpp/include/baml/: - buffer.hpp: RAII OwnedBuffer over the C ABI Buffer (free_buffer on drop) - arg.hpp: tri-state Arg<T> (unset / baml::null / value) for optional arguments, plus the Null unit type - errors.hpp: BamlError / BamlPanic / BamlCancelled; typed thrown-value accessors declared, defined later by the codec - detail/registry.hpp: the one process-global result callback, fanned out to per-call promises via a correlation-id registry; payload copied inside the callback (only valid for its duration) - future.hpp: Future<T> with get/wait/wait_for/cancel, detach-on-destroy; decode deferred to the codec header - runtime.hpp + detail/json.hpp: version(), initialize_runtime from source files (JSON-escaped map); bytecode init pending its C ABI export tests/runtime_smoke.cpp (tests/run.sh): version, runtime init against the real cdylib, registry round trip, Arg semantics, buffer moves - all green. The tarball smoke now also compiles a consumer against include/baml/ and cross-checks baml::version() with the raw C ABI.
Bare null is std::monostate (baml::Null is an alias); explicit null for optional arguments is std::nullopt (Arg<T> constructs from nullopt_t); baml::null / null_t are gone. Union normalization follows: T|null -> std::optional<T>, A|B|null -> std::optional<std::variant<A,B>>, matching Python's Optional[Union[...]] shape with only std types.
A value of the null type is a null: Arg<T> gains a monostate constructor
mapping to the Null state, guarded with enable_if so Arg<std::monostate>
(an optional argument of the bare null type) keeps monostate meaning a
value. Both spellings now work: std::nullopt and baml::Null{}.
# Conflicts: # .github/workflows/cargo-tests.reusable.yaml # baml_language/sdk_tests/harness_runner/src/lib.rs
CMake consumers now add_subdirectory(baml_sdk) and link baml::sdk (a static library over the two generated sources, PUBLIC include path, cxx_std_17, Threads + CMAKE_DL_LIBS for the dlopen/envelope-wait runtime model). Content is static; CMake stays optional and the single-command compile keeps working. Verified end-to-end: a consumer project with add_subdirectory over the type_shapes fixture SDK configures, builds, and completes a real BAML call via BAML_RUNTIME_PATH.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/sdks/cpp/README.md`:
- Around line 19-23: Update the direct C++ build command in the README example
to include the Linux link flags equivalent to CMake’s Threads and
dynamic-loading dependencies, specifically pthread and dl, while preserving the
existing source files and runtime invocation.
🪄 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: 9b9d7307-8467-445c-97cf-b2878d529068
📒 Files selected for processing (2)
baml_language/sdks/cpp/README.mdbaml_language/sdks/cpp/sdkgen_cpp/src/lib.rs
Adds OutputType::Cpp (output_type = "cpp") and the generate.rs dispatch arm: source paths relative to the project root (reference comments only; the payload is bytecode). The CLI's output layout (<output_dir>/baml_sdk/) matches the other generators.
The CMake path links Threads::Threads + CMAKE_DL_LIBS; the direct c++ one-liner needs the equivalent flags on pre-2.34 glibc (std::future, dlopen). No-ops elsewhere.
One constexpr field-number table in proto.h (baml::detail::fields), mirroring the .proto sources in bridge_ctypes/types/. The parser arms, codec encoders, and sdkgen-emitted Codec bodies all reference the named constants, collapsing three independent spellings of the schema to one compile-checked mapping. Values unchanged; wire bytes identical.
The C++ SDK now decodes/encodes through real generated protobuf bindings instead of the hand-rolled reader/writer: - CFFI protos gain optimize_for = LITE_RUNTIME (a no-op for prost/Go); C++ bindings are generated with the repo's vendored protoc (31.1, the same pin bridge_ctypes uses) and checked in under bridge_cpp/pb/ with a drift test + --ignored regenerate bless test (pb_generation.rs) - wire.h deleted; proto.h is now ArgsEncoder over CallFunctionArgs plus Unwrap/ArmName helpers; codec.h decodes typed BamlOutboundValue messages (oneof case switches, literal widening per scalar); errors carry the re-serialized thrown value for get<T>() - consumers build protobuf-lite FROM SOURCE at a pinned version (cmake/fetch_protobuf.cmake: v31.1, protobuf fetches its own pinned abseil, C++ standard pinned to the consumer's so the abseil string_view ABI cannot split). Source-build-by-construction removes the generated-code/runtime version coupling and descriptor-pool collision failure modes; lite has no descriptor pool at all - emitter: Codec bodies emit pb setters/getters; pb sources + the cmake module vendored into baml_sdk; CMakeLists links protobuf::libprotobuf-lite (CMake is now required; README documents the pin, first-build cost, and FETCHCONTENT_SOURCE_DIR_* offline overrides) - harness: cpp_test.sh drives a consumer-shaped CMake shim per fixture (build trees under target/ survive regeneration); setup.sh pre-clones the pinned protobuf/abseil sources once so concurrent configures never race and fixtures need no network; bridge smoke + verify workflow build via CMake (MSVC uses the Debug config so the smoke's asserts stay compiled in) Suite: 10/10 fixtures, bridge smoke, pb drift tests, and a clean-slate end-user demo (baml-cli generate -> cmake -> live call) all green.
|
Deployment failed with the following error: |
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
# Conflicts: # baml_language/Cargo.lock # baml_language/sdk_tests/harness_runner/src/lib.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
baml_language/sdks/cpp/bridge_cpp/cmake/fetch_protobuf.cmake (1)
31-35: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: pin to a commit SHA instead of a mutable tag.
GIT_TAG v31.1pins to a tag, which is technically mutable (could be force-moved on the upstream repo). For a hermetic, reproducible fetch, pinning to the full commit SHA is a stronger guarantee. Not urgent given protocolbuffers/protobuf tags are effectively immutable in practice.🤖 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/cpp/bridge_cpp/cmake/fetch_protobuf.cmake` around lines 31 - 35, Update the protobuf FetchContent_Declare configuration to replace the mutable GIT_TAG v31.1 reference with the full commit SHA corresponding to that release, preserving the existing repository and shallow-fetch settings.
🤖 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/cpp/setup.sh`:
- Around line 26-43: Update the shared checkout initialization around
PROTOBUF_SRC and ABSL_SRC to make cache creation atomic: clone each pinned
repository into a unique temporary directory, then rename it into the final
source path only after a successful clone. Ensure concurrent or interrupted runs
cannot leave an incomplete directory accepted by the existing reuse checks,
while preserving the pinned branches and cache paths.
In `@baml_language/sdk_tests/harness_setup/src/templates/cpp_test.sh`:
- Around line 26-31: Bound the CMake build parallelism in the fixture test
script so concurrent nextest processes do not each launch unrestricted compiler
jobs. Update the build invocations associated with BUILD_DIR for both affected
modes to pass a fixed or configurable job limit to cmake --build, preserving the
existing separate build-tree behavior.
- Around line 71-77: Update the runtime selection case in the MODE=run block to
recognize Windows environments reported by uname, including MSYS, MinGW, or
Cygwin, and set RUNTIME_LIB to bridge_cffi.dll there. Preserve the Darwin dylib
selection and use the existing Unix .so fallback only for non-Windows hosts so
fixture_tests receives the correct runtime path.
In `@baml_language/sdks/cpp/bridge_cpp/include/baml/codec.h`:
- Around line 72-77: Update the kLiteralValue float branch in the codec decode
logic to catch std::invalid_argument and std::out_of_range from std::stod, then
route both failures through KindMismatch so malformed literals surface as
BamlError rather than escaping the decode path.
In `@baml_language/sdks/cpp/bridge_cpp/tests/CMakeLists.txt`:
- Around line 17-21: Update the runtime_smoke target’s target_link_libraries
configuration to include Threads::Threads and ${CMAKE_DL_LIBS} alongside
protobuf::libprotobuf-lite, matching the generated SDK CMake linkage.
In `@baml_language/sdks/cpp/bridge_cpp/tests/run.sh`:
- Around line 2-6: Update the introductory comment in run.sh to describe the
actual cache locations target/cpp-protobuf-src and target/cpp-absl-src, matching
the FETCHCONTENT_SOURCE_DIR_PROTOBUF and FETCHCONTENT_SOURCE_DIR_ABSL
configuration used by the script; remove the nonexistent target/cpp-fetchcontent
reference.
---
Nitpick comments:
In `@baml_language/sdks/cpp/bridge_cpp/cmake/fetch_protobuf.cmake`:
- Around line 31-35: Update the protobuf FetchContent_Declare configuration to
replace the mutable GIT_TAG v31.1 reference with the full commit SHA
corresponding to that release, preserving the existing repository and
shallow-fetch settings.
🪄 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: 0824fc63-6b38-4dda-bfa4-28e480e4caa8
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.github/workflows/cargo-tests.reusable.yaml.github/workflows/verify-cpp-sdk.reusable.yamlbaml_language/.config/nextest.tomlbaml_language/Cargo.tomlbaml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_handle.protobaml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_inbound.protobaml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_outbound.protobaml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_type.protobaml_language/scripts/fmt_cpp.shbaml_language/sdk_tests/crates/cpp/setup.shbaml_language/sdk_tests/harness_runner/src/lib.rsbaml_language/sdk_tests/harness_setup/src/lib.rsbaml_language/sdk_tests/harness_setup/src/templates/cpp_test.shbaml_language/sdks/cpp/README.mdbaml_language/sdks/cpp/bridge_cpp/cmake/fetch_protobuf.cmakebaml_language/sdks/cpp/bridge_cpp/include/baml/codec.hbaml_language/sdks/cpp/bridge_cpp/include/baml/detail/proto.hbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_handle.pb.ccbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_handle.pb.hbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_inbound.pb.ccbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_inbound.pb.hbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_outbound.pb.ccbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_outbound.pb.hbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_type.pb.ccbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_type.pb.hbaml_language/sdks/cpp/bridge_cpp/tests/CMakeLists.txtbaml_language/sdks/cpp/bridge_cpp/tests/run.shbaml_language/sdks/cpp/sdkgen_cpp/Cargo.tomlbaml_language/sdks/cpp/sdkgen_cpp/src/lib.rsbaml_language/sdks/cpp/sdkgen_cpp/tests/pb_generation.rsbaml_language/sdks/python/src/baml_bridge/cffi/v1/baml_handle_pb2.pybaml_language/sdks/python/src/baml_bridge/cffi/v1/baml_inbound_pb2.pybaml_language/sdks/python/src/baml_bridge/cffi/v1/baml_outbound_pb2.pybaml_language/sdks/python/src/baml_bridge/cffi/v1/baml_type_pb2.py
🚧 Files skipped from review as they are similar to previous changes (6)
- baml_language/sdk_tests/harness_runner/src/lib.rs
- baml_language/Cargo.toml
- baml_language/.config/nextest.toml
- .github/workflows/cargo-tests.reusable.yaml
- .github/workflows/verify-cpp-sdk.reusable.yaml
- baml_language/sdks/cpp/sdkgen_cpp/src/lib.rs
# Conflicts: # .github/workflows/cargo-tests.reusable.yaml # .github/workflows/release-baml-language.yml # baml_language/Cargo.lock # baml_language/crates/baml_cli/src/generate.rs # baml_language/sdk_tests/harness_runner/src/lib.rs # release/platforms.json # scripts/baml-release-manifests
- regenerate the Go and TypeScript proto bindings (the LITE_RUNTIME option changed every descriptor; Rust/Python were already refreshed) - drop the stale platform-contract drift test in baml_release::manifest: it read the pre-#4045 platforms.json schema, and canary's baml_release::platforms module now owns the same check against the new schema - cap cmake build parallelism in the harness driver and bridge smoke (bare -j is unbounded with Makefiles; eight concurrent fixture builds starved the CI runner to death) and embed the vendored pb sources gzipped (build.rs): baml-cli carries every generator, and 1.9 MB of plain protobuf text blew the 3% size gate; compressed it is +150 KB (+2.0% measured, back under the gate)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/release-baml-language.yml (1)
797-801: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider env-var indirection for
crates_io_versioninterpolation (zizmor template-injection hint).
args+=(--crates-io-version "${{ needs.plan.outputs.crates_io_version }}")interpolates a job output directly into therun:script rather than via anenv:variable. The value is an internally-computed semver (not attacker input), so practical risk is low, and this mirrors the existing style used throughout the file (e.g.version,channelabove). Also applies to lines 1011-1013 indry-run-artifacts.♻️ Suggested pattern
+ env: + CRATES_IO_VERSION: ${{ needs.plan.outputs.crates_io_version }} run: | ... - args+=(--crates-io-version "${{ needs.plan.outputs.crates_io_version }}") + args+=(--crates-io-version "$CRATES_IO_VERSION")🤖 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/release-baml-language.yml around lines 797 - 801, Update the release workflow’s crates_io_version interpolation in both the publish and dry-run artifact paths to pass the job output through an env variable, then reference that variable in the shell args construction. Preserve the existing success condition and --crates-io-version behavior.Source: Linters/SAST tools
🤖 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 @.github/workflows/release-baml-language.yml:
- Around line 797-801: Update the release workflow’s crates_io_version
interpolation in both the publish and dry-run artifact paths to pass the job
output through an env variable, then reference that variable in the shell args
construction. Preserve the existing success condition and --crates-io-version
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3ee3a8b5-cf55-40bf-a50e-5d45dbfba643
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.github/workflows/cargo-tests.reusable.yaml.github/workflows/release-baml-language.yml.github/workflows/verify-cpp-sdk.reusable.yamlbaml_language/.config/nextest.tomlbaml_language/Cargo.tomlbaml_language/crates/baml_cli/Cargo.tomlbaml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_codegen_types/src/generator_fields.rsbaml_language/crates/baml_release/src/manifest.rsbaml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_handle.protobaml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_inbound.protobaml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_outbound.protobaml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_type.protobaml_language/scripts/fmt_cpp.shbaml_language/sdk_tests/crates/cpp/setup.shbaml_language/sdk_tests/harness_runner/src/lib.rsbaml_language/sdk_tests/harness_setup/Cargo.tomlbaml_language/sdk_tests/harness_setup/src/lib.rsbaml_language/sdk_tests/harness_setup/src/templates/cpp_test.shbaml_language/sdks/cpp/README.mdbaml_language/sdks/cpp/bridge_cpp/cmake/fetch_protobuf.cmakebaml_language/sdks/cpp/bridge_cpp/include/baml/codec.hbaml_language/sdks/cpp/bridge_cpp/include/baml/detail/proto.hbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_handle.pb.ccbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_handle.pb.hbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_inbound.pb.ccbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_inbound.pb.hbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_outbound.pb.ccbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_outbound.pb.hbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_type.pb.ccbaml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_type.pb.hbaml_language/sdks/cpp/bridge_cpp/tests/CMakeLists.txtbaml_language/sdks/cpp/bridge_cpp/tests/run.shbaml_language/sdks/cpp/sdkgen_cpp/Cargo.tomlbaml_language/sdks/cpp/sdkgen_cpp/src/lib.rsbaml_language/sdks/cpp/sdkgen_cpp/tests/pb_generation.rsbaml_language/sdks/python/src/baml_bridge/cffi/v1/baml_handle_pb2.pybaml_language/sdks/python/src/baml_bridge/cffi/v1/baml_inbound_pb2.pybaml_language/sdks/python/src/baml_bridge/cffi/v1/baml_outbound_pb2.pybaml_language/sdks/python/src/baml_bridge/cffi/v1/baml_type_pb2.pyscripts/baml-release-manifests
🚧 Files skipped from review as they are similar to previous changes (29)
- baml_language/sdks/cpp/sdkgen_cpp/Cargo.toml
- baml_language/sdks/cpp/bridge_cpp/tests/run.sh
- baml_language/scripts/fmt_cpp.sh
- baml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_outbound.proto
- baml_language/sdk_tests/harness_setup/src/lib.rs
- baml_language/crates/baml_codegen_types/src/generator_fields.rs
- baml_language/sdk_tests/harness_runner/src/lib.rs
- baml_language/sdks/cpp/bridge_cpp/tests/CMakeLists.txt
- baml_language/sdks/python/src/baml_bridge/cffi/v1/baml_type_pb2.py
- baml_language/sdks/cpp/sdkgen_cpp/tests/pb_generation.rs
- baml_language/Cargo.toml
- baml_language/crates/baml_cli/src/generate.rs
- baml_language/sdks/cpp/bridge_cpp/cmake/fetch_protobuf.cmake
- baml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_inbound.pb.h
- baml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_type.proto
- baml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_handle.proto
- baml_language/crates/bridge_ctypes/types/baml_bridge/cffi/v1/baml_inbound.proto
- baml_language/sdks/cpp/README.md
- baml_language/sdks/python/src/baml_bridge/cffi/v1/baml_outbound_pb2.py
- baml_language/crates/baml_release/src/manifest.rs
- baml_language/.config/nextest.toml
- .github/workflows/verify-cpp-sdk.reusable.yaml
- .github/workflows/cargo-tests.reusable.yaml
- baml_language/sdks/cpp/bridge_cpp/include/baml/detail/proto.h
- baml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_type.pb.h
- baml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_handle.pb.h
- baml_language/sdks/cpp/bridge_cpp/include/baml/codec.h
- baml_language/sdks/cpp/bridge_cpp/pb/baml_bridge/cffi/v1/baml_outbound.pb.h
- baml_language/sdks/cpp/sdkgen_cpp/src/lib.rs
Drop canonicalize (it yields a \\?\ extended-length root protoc cannot relate proto files to) and pass absolute proto file paths under --proto_path, matching bridge_ctypes/build.rs.
A fan-out audit comparing every C++ sdk test with its python original
found five weaker-assertion gaps (no behavioral divergences). Fixes:
- type_shapes/test_main.cc names one symbol from EVERY in-scope
generated namespace (python: test_all_namespaces_reachable);
complex_models and unions had zero references anywhere in the
fixture, so codegen dropping them would have passed CI
- docstrings: Doc/Note/Sentiment doc blocks are now anchored on the
namespace open (nothing may precede the summary line, matching
python's exact __doc__ equality); Note additionally pins the blank
separator line before Attributes: as one contiguous block
- docstrings: Priority's summary-only case now asserts the class-level
summary POSITIVELY (it only asserted Members: absence), pins exactly
three enumerators, and checks the wire values via Codec::ToWire
(python: doc equality + {m.value} set equality)
- setup.sh/run.sh: pinned source clones are atomic (temp dir + rename), so an interrupted clone cannot leave a half-populated cache the -d guard would accept - cpp_test.sh: recognize MSYS/MinGW/Cygwin unames and select bridge_cffi.dll (forward-looking; the cpp matrix is unix-only today) - codec.h: malformed float-literal text surfaces as BamlError instead of a bare std::invalid_argument escaping the decode path - tests/CMakeLists.txt: link Threads::Threads + CMAKE_DL_LIBS, matching the generated SDK's platform deps - run.sh: fix a stale comment referencing the removed FetchContent cache path
This was referenced Jul 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Packaging slice of the C++ bridge (bridge-week, Implement bridge-cpp): make the
bridge_cffiC ABI consumable as a classiclib/+include/artifact and wire it into the nightly release graph, before any codegen work.What
ABI header (committed, guarded)
crates/bridge_cffi/include/baml_cffi.h, generated by cbindgen and committed as the reviewed ABI contract. A freshness test (tests/header_is_current.rs) regenerates in-memory and diffs, so any FFI surface change fails CI until the header is regenerated (BLESS=1 cargo test -p bridge_cffi --test header_is_current) and reviewed. cbindgen version-pinned (0.29.4 dev-dep).cpp_compat(extern "C" guards), include guard, enum variant prefixing (Ok->BamlCffiStatus_Ok), and parsing ofsys_native/bex_resource_typesso theHostDispatchFn/HostReleaseFntypedefs are emitted (previously the header would not have compiled).Packaging + smoke test
mise run package-cpp: builds the cdylib and assemblesbaml-cpp-<version>-<target>/{lib,include,VERSION,LICENSE}+ sha256 sidecar. Auto-includes header-onlybridge_cppheaders oncesdks/cpp/bridge_cpp/include/baml/exists.mise run smoke-cpp: consumes the tarball exactly as a user would - temp dir, C++17 compile againstinclude/+lib/only, run, assertversion()round-trips. (C++17 is the locked minimum standard.)release-bridge-cffiprofile (release +panic=unwind):[profile.release]ispanic=abort, which would SIGABRT the host process on any engine panic instead of delivering a catchable BamlPanic - same reasoning asrelease-bridge-python/release-bridge-node.Release graph wiring
build2-cpp-sdk.reusable.yaml: 8-target matrix mirroring the toolchain build (native runners, musl-tools, MSVC cross for aarch64 windows); stamps the frozen release plan, packages, smoke-tests on unix targets, uploadscpp-sdk-<target>artifacts.release-baml-language.yml:build-cpp-sdkjob wired like the python/node SDK builds; tarballs attach to thebaml-language-<version>GitHub release; production and dry-run manifest jobs collect them via--cpp-dir.baml-release-manifests: newbaml_cppmanifest key with URLs pointing at the baml-language release tag the files are uploaded to.baml_release: optionalToolchainManifest.baml_cppfield - schema stays 1, older wrappers ignore it - validated against the exact release target set, with round-trip tests.Test plan
cargo test -p bridge_cffi --test header_is_currentgreen; header syntax-checked standalone as C11 and C++17mise run package-cpp && mise run smoke-cppon aarch64-apple-darwin:smoke test passed: version 0.14.1(with the release-bridge-cffi profile)cargo test -p baml_release: 23 passed (4 new manifest tests)baml-release-manifestsexercised end-to-end with fake artifacts for all 8 targets; output JSON has the fullbaml_cppblock with correct release-tag URLsgh workflow run release-baml-language.yml -f channel=nightly -f dry_run=true) exercises the cpp matrix with the frozen plan; the workflow only becomes dispatchable once it exists on canarySummary by CodeRabbit
cppoutput target, producing typed APIs with embedded bytecode and a companion C++ runtime bridge interface.