Repository navigation
Conversation
Register Ruby as a permanent bridge language and add the private FFI loader with ABI validation, lifecycle safety, callback retention, fork protection, and exact-byte initialization. Seed the Ruby/Sorbet harness in the existing cross-platform SDK-test matrix and extend version stamping and ABI assertions.
|
@ryanmazzolini is attempting to deploy a commit to the Boundary Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a private Ruby bridge with CFFI-based runtime loading, Ruby bridge identity, version management, native fixtures, lifecycle tests, and Linux, macOS, and Windows CI coverage. ChangesRuby bridge contracts
Ruby runtime
Native fixtures and tests
CI wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: ⚪ Minimal · up to The PR adds Ruby bridge registration and process-wide runtime loading with ABI, concurrency, and fork-safety handling; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant RubyTest
participant BamlBridge
participant ProcessRuntime
participant NativeAPI
RubyTest->>BamlBridge: initialize!(compiled_program_bytes)
BamlBridge->>ProcessRuntime: initialize program
ProcessRuntime->>NativeAPI: load and validate API table
NativeAPI-->>ProcessRuntime: API and runtime status
ProcessRuntime->>NativeAPI: register Ruby metadata and callback
ProcessRuntime->>NativeAPI: initialize bytecode
NativeAPI-->>RubyTest: success or bridge error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 67ce4bc2e6
ℹ️ 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".
| "timeout": 30, | ||
| }, | ||
| { | ||
| "sdk-label": "ruby-sorbet", |
There was a problem hiding this comment.
In the sdk-test-coverage job, covered directories are derived solely from each matrix entry's sdk-dir, but all three Ruby entries omit that property. Since this commit also adds baml_language/sdks/ruby, the coverage check will always include ruby in missingSdkDirs and call core.setFailed, breaking this workflow on every event; add "sdk-dir": "ruby" to each Ruby entry.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/tests/test_baml_language_version.py (1)
251-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify both Ruby version constants in the stamp test.
make_fixturewritesTOOLCHAIN_VERSIONandBRIDGE_RUNTIME_VERSION.surface_versions()reads onlyTOOLCHAIN_VERSION. The nightly test also checks only that value. The latersyncoperation can overwrite a stale runtime version beforecheckruns. AddBRIDGE_RUNTIME_VERSIONto the extracted and asserted values.Suggested test update
"ruby": match( "baml_language/sdks/ruby/bridge_ruby/lib/baml/bridge/version.rb", r'^ TOOLCHAIN_VERSION = "([^"]+)"$', ), + "ruby_runtime": match( + "baml_language/sdks/ruby/bridge_ruby/lib/baml/bridge/version.rb", + r'^ BRIDGE_RUNTIME_VERSION = "([^"]+)"$', + ), ... - for sdk in ("node", "web", "rust", "go", "ruby", "csharp", "vsix"): + for sdk in ( + "node", + "web", + "rust", + "go", + "ruby", + "ruby_runtime", + "csharp", + "vsix", + ):Also applies to: 310-310
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_baml_language_version.py` around lines 251 - 254, Update the Ruby entry in the stamp test’s version extraction and assertion flow around make_fixture, surface_versions(), and the nightly check to capture both TOOLCHAIN_VERSION and BRIDGE_RUNTIME_VERSION from version.rb. Ensure the later sync/check sequence validates each constant independently, so a stale BRIDGE_RUNTIME_VERSION cannot be overwritten without detection.baml_language/sdks/ruby/bridge_ruby/lib/baml/bridge/process_runtime.rb (1)
112-120: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGive the result queue and the callback error queue a drain path, or bound them.
receive_resultappends to@result_eventswithout a bound.pop_result_eventis private and unused, so nothing drains it. The same applies toBorrowedBytesCallback#pop_errorinnative.rbat lines 245-249:load_api!retains the callback but never inspects its error queue. Once the native runtime delivers results, the process retains every event forever, and every exception raised inside the callback stays invisible.This checkpoint does not call
call_function, so no growth occurs yet. Add a short comment that records the intended consumer, or drain both queues, before result delivery lands.🤖 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/ruby/bridge_ruby/lib/baml/bridge/process_runtime.rb` around lines 112 - 120, Document the intended consumers for both unbounded queues before result delivery is added: add a brief comment near `receive_result`/`pop_result_event` identifying the future result-event consumer, and near `BorrowedBytesCallback#pop_error` identifying how callback errors will be drained or surfaced. Do not alter queue behavior or add unrelated runtime handling.baml_language/sdk_tests/crates/ruby_sorbet/setup.sh (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth setup scripts assume the cargo artifact location. Each script hardcodes the workspace
targetdirectory and thedebugprofile, soBAML_RUBY_TEST_REAL_RUNTIMEbreaks whenCARGO_TARGET_DIRis set or when the lane builds with--release.
baml_language/sdk_tests/crates/ruby_sorbet/setup.sh#L7-L7: setTARGET_DIR="${CARGO_TARGET_DIR:-$WORKSPACE_ROOT/target}", and derive the profile directory used at lines 25 and 30 instead of hardcodingdebug.baml_language/sdk_tests/crates/ruby_sorbet/setup.ps1#L4-L6: read$env:CARGO_TARGET_DIRwhen it is set, and derive the profile directory used at line 58 instead of hardcodingdebug.🤖 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/ruby_sorbet/setup.sh` at line 7, Update both ruby_sorbet setup scripts to honor Cargo’s configured artifact location and build profile: in baml_language/sdk_tests/crates/ruby_sorbet/setup.sh lines 7, 25, and 30, use CARGO_TARGET_DIR when set and derive the profile directory instead of assuming debug; in baml_language/sdk_tests/crates/ruby_sorbet/setup.ps1 lines 4-6 and 58, use $env:CARGO_TARGET_DIR when set and derive the corresponding profile directory instead of hardcoding debug.
🤖 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 526-535: Add the sdk-dir field with value "ruby" to every Ruby
matrix entry, including the Linux entry at
.github/workflows/cargo-tests.reusable.yaml lines 526-535, the macOS entry at
lines 633-642, and the Windows entry at lines 745-754, so coverage discovery
includes the Ruby SDK directory.
In `@baml_language/sdks/ruby/bridge_ruby/lib/baml/bridge/native.rb`:
- Around line 111-124: Update read_table to check whether
find_function("baml_get_api_v1") returns nil before constructing FFI::Function,
and raise IncompatibleRuntimeError with the symbol-specific diagnostic used for
an unresolved baml_get_api_v1 entry. Keep the existing null-pointer validation
and FFI::NotFoundError handling unchanged.
---
Nitpick comments:
In `@baml_language/sdk_tests/crates/ruby_sorbet/setup.sh`:
- Line 7: Update both ruby_sorbet setup scripts to honor Cargo’s configured
artifact location and build profile: in
baml_language/sdk_tests/crates/ruby_sorbet/setup.sh lines 7, 25, and 30, use
CARGO_TARGET_DIR when set and derive the profile directory instead of assuming
debug; in baml_language/sdk_tests/crates/ruby_sorbet/setup.ps1 lines 4-6 and 58,
use $env:CARGO_TARGET_DIR when set and derive the corresponding profile
directory instead of hardcoding debug.
In `@baml_language/sdks/ruby/bridge_ruby/lib/baml/bridge/process_runtime.rb`:
- Around line 112-120: Document the intended consumers for both unbounded queues
before result delivery is added: add a brief comment near
`receive_result`/`pop_result_event` identifying the future result-event
consumer, and near `BorrowedBytesCallback#pop_error` identifying how callback
errors will be drained or surfaced. Do not alter queue behavior or add unrelated
runtime handling.
In `@scripts/tests/test_baml_language_version.py`:
- Around line 251-254: Update the Ruby entry in the stamp test’s version
extraction and assertion flow around make_fixture, surface_versions(), and the
nightly check to capture both TOOLCHAIN_VERSION and BRIDGE_RUNTIME_VERSION from
version.rb. Ensure the later sync/check sequence validates each constant
independently, so a stale BRIDGE_RUNTIME_VERSION cannot be overwritten without
detection.
🪄 Autofix
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 Plus
Run ID: 1e368d81-656e-421e-ae08-51e5230a8ab2
⛔ Files ignored due to path filters (2)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/sdk_tests/crates/ruby_sorbet/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/cargo-tests.reusable.yamlbaml_language/.config/nextest.tomlbaml_language/Cargo.tomlbaml_language/crates/bridge_cffi/include/baml_cffi.hbaml_language/crates/bridge_cffi/src/ffi/runtime.rsbaml_language/crates/bridge_cffi/src/identity.rsbaml_language/crates/bridge_cffi/tests/abi_assertions.hbaml_language/crates/bridge_cffi/tests/abi_layout.rsbaml_language/sdk_tests/crates/ruby_sorbet/Cargo.tomlbaml_language/sdk_tests/crates/ruby_sorbet/Gemfilebaml_language/sdk_tests/crates/ruby_sorbet/setup.ps1baml_language/sdk_tests/crates/ruby_sorbet/setup.shbaml_language/sdk_tests/crates/ruby_sorbet/src/lib.rsbaml_language/sdk_tests/crates/ruby_sorbet/test/bridge_loader_test.rbbaml_language/sdk_tests/crates/ruby_sorbet/test/bridge_scenario.rbbaml_language/sdk_tests/crates/ruby_sorbet/test/native/bridge_fixture.cbaml_language/sdk_tests/crates/ruby_sorbet/test/native/missing_getter.cbaml_language/sdk_tests/crates/ruby_sorbet/test/native/thread_callback.cppbaml_language/sdk_tests/harness_setup/src/lib.rsbaml_language/sdks/go/baml_go/internal/cffi/include/baml_cffi.hbaml_language/sdks/ruby/bridge_ruby/Gemfilebaml_language/sdks/ruby/bridge_ruby/README.mdbaml_language/sdks/ruby/bridge_ruby/lib/baml/bridge.rbbaml_language/sdks/ruby/bridge_ruby/lib/baml/bridge/errors.rbbaml_language/sdks/ruby/bridge_ruby/lib/baml/bridge/native.rbbaml_language/sdks/ruby/bridge_ruby/lib/baml/bridge/process_runtime.rbbaml_language/sdks/ruby/bridge_ruby/lib/baml/bridge/version.rbbaml_language/sdks/swift/Sources/CBamlBridge/include/baml_cffi.hscripts/baml-language-versionscripts/tests/test_baml_language_version.py
Ensure CI covers the Ruby SDK and missing native symbols fail with a stable diagnostic. Keep version stamps, callback ownership, target-directory handling, and Windows fixture compilation aligned across platforms.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0cf568338
ℹ️ 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".
| candidate_path = nil | ||
| if @api.nil? && @terminal_error.nil? | ||
| candidate_path = configured_runtime_path! | ||
| claim_process! |
There was a problem hiding this comment.
When BAML_RUNTIME_PATH names an existing but unloadable file, claim_process! records the current PID before load_api! raises the documented retryable RuntimeLoadError, and that claim is never cleared. If a prefork application subsequently forks and corrects the path in a worker, the worker receives ForkSafetyError even though no native library or runtime state was successfully loaded. Reset the ownership claim after an open failure, or otherwise distinguish failed pre-load attempts from native use.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9891fa9: a failed library open now releases the PID claim because no native state was created. The loader scenario now forks after that failure and proves the child can correct the path. The focused suite passes with 9 tests and 41 assertions.
Addressed with AI assistance.
Allow prefork workers to correct an unloadable runtime path because a failed library open creates no native state. Cover the retry in a forked child before the parent retries.
|
Closed the three earlier summary-only review notes in
Focused validation passed, and CodeRabbit’s follow-up review approved the updated head. Addressed with AI assistance. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9891fa9dc0
ℹ️ 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".
| # Opening a file does not create native state. Release the process | ||
| # claim so a prefork worker can correct the path and retry. | ||
| @owner_pid = nil |
There was a problem hiding this comment.
When two threads enter while @api is unset, both can execute claim_process!; if the first thread's library open fails while the second is waiting with a valid candidate_path, this unconditional reset clears their shared PID claim. The second thread can then load and initialize the native runtime without restoring @owner_pid, so a later fork is incorrectly accepted and may use inherited native state. Clear the claim only when no other initialization attempt owns it, or re-claim inside the mutex before loading.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6b755e5: each thread now reclaims process ownership inside the serialized load section, after any earlier initializer has finished and possibly released a failed-open claim. The deterministic two-thread regression forces the reported ordering, verifies the surviving initializer owns the PID, and proves a later child is rejected. The focused suite passes with 9 tests and 42 assertions.
Addressed with AI assistance.
Reclaim process ownership inside the serialized load path when another initializer releases a failed-open claim. Add a deterministic concurrent regression that verifies the surviving initializer restores fork protection.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b755e54b6
ℹ️ 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".
| rescue LoadError => error | ||
| # Opening a file does not create native state. Release the process | ||
| # claim so a prefork worker can correct the path and retry. | ||
| @owner_pid = nil |
There was a problem hiding this comment.
If another thread forks after this assignment but before the enclosing @mutex.synchronize unwinds, the child sees no owner at the initial fork check and then blocks forever on the mutex inherited from the vanished loader thread. The newly added re-claim at line 46 does not close this window because it runs only after acquiring that mutex; release the claim only once the mutex is unlocked, while ensuring a concurrent successful initializer cannot have its claim cleared.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 03b61d8: failed opens now keep the PID claim, while per-attempt tokens remain present until synchronize has released the mutex. A child can reclaim only after every attempt completed without creating native state.
The new regression forks while a failed open still holds the initialization section, verifies the child rejects promptly, then verifies recovery after the parent unlocks. This model also supersedes the earlier clear-and-reclaim fixes. The focused suite passes with 9 tests and 43 assertions.
Addressed with AI assistance.
Keep process ownership after an unloadable runtime and track active initialization attempts until their mutex is released. A forked child can reclaim only when the failed attempt completed without creating native state.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03b61d839e
ℹ️ 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".
| ensure | ||
| # Remove this token only after synchronize has released the mutex. | ||
| @initialization_attempts.delete(attempt) if attempt |
There was a problem hiding this comment.
If another thread forks after @mutex.synchronize has unlocked but before this ensure deletes the attempt token, the child permanently inherits a nonempty @initialization_attempts array whose originating thread no longer exists. Although the library open failed and no native state exists, every later initialize! in that child therefore raises ForkSafetyError instead of permitting the documented corrected-path retry. The new token cleanup is fresh evidence of a post-unlock race distinct from the previously reported locked-mutex window; recovery should account for the mutex already being unlocked rather than relying solely on the inherited token being removed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a34fb72: fork recovery no longer depends on an inherited attempt token. The runtime now tracks only the active native library open. A child rejects while that native operation is active, then replaces the inherited Ruby mutex and retries once a failed open has returned without native state.
The regression now covers both sides of the boundary: it rejects a child while the open is active, then successfully retries from another child while the parent still holds the post-failure Ruby mutex. This removes the post-unlock token window entirely. The focused suite passes with 9 tests and 43 assertions.
Addressed with AI assistance.
Track the native library open directly instead of inferring it from initialization tokens. A child rejects during dlopen, then replaces the inherited Ruby mutex and retries once the failed open has returned without native state.
sxlijin
changed the title
|
Sorry for the delay on this! |
3 participants
Description
Ruby needs a safe process-wide loader before the Ruby/Sorbet SDK can call the BAML 1.0 runtime.
This change registers Ruby as a permanent bridge language and adds the private Ruby FFI loader. The loader validates the V1 ABI, initializes exact compiled-program bytes once per process, and fails safely for incompatible runtimes and unsupported post-use forks.
Details
10.libbridge_cffionly throughBAML_RUNTIME_PATH.sdk_test_ruby_sorbetin the existing Linux, macOS, and Windows SDK-test lanes without adding a Ruby-specific workflow or version matrix.Shortcut: https://app.shortcut.com/odeko/story/64730
Testing
Passed locally on Apple Silicon macOS:
cargo nextest run -p bridge_cffi --all-features: 43 passed, 1 ignoredcargo nextest run -p sdk_test_ruby_sorbet --all-featurespython3 -m unittest scripts.tests.test_baml_language_version: 13 passedscripts/baml-language-version checkcargo fmt --all -- --checkactionlint .github/workflows/cargo-tests.reusable.yamlshellcheckandbash -nforsetup.shThe full workspace regression suite was deferred to CI after local Rust build output grew to 33.6 GiB. Linux and Windows execution also remains for CI.
Developed with AI assistance.
Summary by CodeRabbit
New Features
Bug Fixes
Tests