Repository navigation
bridge web - #3966
bridge web#3966sxlijin wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe workflow adds merge-queue reuse validation, while the Rust workspace gains a WASM bridge scaffold and a generated TypeScript web SDK test harness with cross-platform setup and browser-oriented validation. ChangesWeb bridge and TypeScript web SDK
Merge-queue CI fast path
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![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: 7fd71fa6f2
ℹ️ 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".
| statuses: write | ||
| steps: | ||
| - name: Report failure | ||
| if: contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') |
There was a problem hiding this comment.
If determine_changes fails (for example checkout, merge-base, or a broken path-filter script), every direct job listed in needs is skipped, so this condition sees only skipped results and the required ci-failure-alert job passes; on PRs the following marker step can then record the tree as green even though no CI ran. The previous failure() || cancelled() job condition caught failed ancestors, so this gate needs to include determine_changes/other ancestors or use a failure status check here instead of only inspecting direct needs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
76-79: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: page the combined-status lookup.
getCombinedStatusForRefreturns at most 30 statuses per page by default, so if the PR head accumulates more than 30 commit statuses,findcould miss theci-fast-path/baml-languagemarker (fail-open → full CI). Only commit statuses count here (not Actions check runs), so risk is low, but aper_pagebump makes the fast path robust.♻️ Suggested tweak
const { data: combined } = await github.rest.repos.getCombinedStatusForRef({ - owner, repo, ref: prHead, + owner, repo, ref: prHead, per_page: 100, });🤖 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/ci.yaml around lines 76 - 79, Make the combined-status lookup in the PR head status-check logic robust against pagination by requesting a larger page size via the getCombinedStatusForRef options, then continue locating the ci-fast-path/baml-language marker from combined.statuses. Preserve the existing fail-open behavior if the marker is absent.
🤖 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/ci.yaml:
- Around line 76-79: Make the combined-status lookup in the PR head status-check
logic robust against pagination by requesting a larger page size via the
getCombinedStatusForRef options, then continue locating the
ci-fast-path/baml-language marker from combined.statuses. Preserve the existing
fail-open behavior if the marker is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1ee202fc-7146-4dfc-b53c-4b4b409c03c7
📒 Files selected for processing (1)
.github/workflows/ci.yaml
7fd71fa to
b8451bf
Compare
b8451bf to
b4b0211
Compare
⏭️ 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):
|
Binary size checks passed✅ 7 passed
Generated by |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
baml_language/crates/bridge_cffi/src/wasm.rs (1)
15-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the WASM bridge transitions. Cover empty-bytecode rejection, the pre/post-staging
call_function_in_wasmbehavior, and monotonicnew_function_call_id()allocation.🤖 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/bridge_cffi/src/wasm.rs` around lines 15 - 35, Add unit tests for stage_runtime_bytecode, call_function_in_wasm, and new_function_call_id: verify empty bytecode is rejected, calls return the uninitialized error before staging and WASM_RUNTIME_UNAVAILABLE after successful staging, and successive call IDs increase monotonically. Isolate or reset shared atomic state as needed so tests remain deterministic.Source: Coding guidelines
baml_language/sdk_tests/harness_setup/src/typescript_web.rs (2)
156-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider unit tests for the pure recursion/formatting helpers.
has_vitest_tests,rewrite_test_bridge_imports, andwrite_fixtures_tests_rscontain non-trivial recursion/exclusion logic (e.g., thebaml_sdkskip,.test.tssuffix matching) with no accompanying unit tests in this diff.As per coding guidelines, "Prefer writing Rust unit tests over integration tests where possible" — these are good candidates for
#[cfg(test)]unit tests using atempfile-based fixture tree.🤖 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/harness_setup/src/typescript_web.rs` around lines 156 - 169, Add #[cfg(test)] unit tests for has_vitest_tests, rewrite_test_bridge_imports, and write_fixtures_tests_rs using tempfile-based fixture trees. Cover recursive discovery, .test.ts suffix matching, baml_sdk exclusion, and the expected import rewriting and generated fixture output formatting.Source: Coding guidelines
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd context to
.unwrap()on required env vars.
CARGO_MANIFEST_DIR/OUT_DIRare unwrapped with no message, while the sibling helper inharness_runner.rsuses a descriptive.expect("CARGO_MANIFEST_DIR not set; run via \cargo test`")` for the same variable. A bare panic here gives a confusing build-script failure.♻️ Proposed fix
- let manifest_dir = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap()); + let manifest_dir = PathBuf::from( + env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR not set; run via `cargo build`"), + ); let fixtures_root = fixtures_root_from_manifest(&manifest_dir); let node_tests_root = manifest_dir.parent().unwrap().join("typescript_node"); - let out_dir = PathBuf::from(env::var("OUT_DIR").unwrap()); + let out_dir = PathBuf::from(env::var("OUT_DIR").expect("OUT_DIR not set; run via `cargo build`"));🤖 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/harness_setup/src/typescript_web.rs` around lines 22 - 27, Update the environment-variable reads in run_all to replace the bare unwrap calls for CARGO_MANIFEST_DIR and OUT_DIR with descriptive expect messages. Match the existing harness_runner.rs wording for CARGO_MANIFEST_DIR and provide equivalent context for OUT_DIR.baml_language/sdk_tests/harness_runner/src/lib.rs (1)
207-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeb ESM assertion is weaker than its Node counterpart.
assert_typescript_node_generated_esm(lines 177-186) also asserts absence of CommonJS syntax (module.exports,exports.,require() and presence of ESMimport/exportsyntax.assert_typescript_web_generated_esmdrops both checks, only validating bridge-package usage and import extensions.tsc/vitestwould likely still catch a real regression, but the earlier, clearer signal from this assertion is lost.🤖 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/harness_runner/src/lib.rs` around lines 207 - 258, Strengthen assert_typescript_web_generated_esm to match assert_typescript_node_generated_esm by rejecting generated CommonJS syntax (module.exports, exports., and require() calls) and requiring ESM import/export syntax. Apply these checks while scanning each generated SDK file, preserving the existing web bridge and relative-import extension validations.
🤖 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/typescript_web/setup.ps1`:
- Around line 16-29: Update the fixture enumeration in the setup script to scan
the generated packages under the harness_setup fixture root rather than the
crate directory. Preserve the existing pnpm install/update and exit handling for
each generated directory, and only write SDK_TEST_TYPESCRIPT_WEB_SETUP after
those fixtures have been processed.
In `@baml_language/sdk_tests/harness_setup/src/templates/package_web.json`:
- Line 10: Update the `@boundaryml/baml-bridge-web` dependency path in
package_web.json to resolve to baml_language/sdks/web/bridge_web, correcting the
relative path so generated fixtures reference the intended bridge package.
---
Nitpick comments:
In `@baml_language/crates/bridge_cffi/src/wasm.rs`:
- Around line 15-35: Add unit tests for stage_runtime_bytecode,
call_function_in_wasm, and new_function_call_id: verify empty bytecode is
rejected, calls return the uninitialized error before staging and
WASM_RUNTIME_UNAVAILABLE after successful staging, and successive call IDs
increase monotonically. Isolate or reset shared atomic state as needed so tests
remain deterministic.
In `@baml_language/sdk_tests/harness_runner/src/lib.rs`:
- Around line 207-258: Strengthen assert_typescript_web_generated_esm to match
assert_typescript_node_generated_esm by rejecting generated CommonJS syntax
(module.exports, exports., and require() calls) and requiring ESM import/export
syntax. Apply these checks while scanning each generated SDK file, preserving
the existing web bridge and relative-import extension validations.
In `@baml_language/sdk_tests/harness_setup/src/typescript_web.rs`:
- Around line 156-169: Add #[cfg(test)] unit tests for has_vitest_tests,
rewrite_test_bridge_imports, and write_fixtures_tests_rs using tempfile-based
fixture trees. Cover recursive discovery, .test.ts suffix matching, baml_sdk
exclusion, and the expected import rewriting and generated fixture output
formatting.
- Around line 22-27: Update the environment-variable reads in run_all to replace
the bare unwrap calls for CARGO_MANIFEST_DIR and OUT_DIR with descriptive expect
messages. Match the existing harness_runner.rs wording for CARGO_MANIFEST_DIR
and provide equivalent context for OUT_DIR.
🪄 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: 55003535-9f56-4df8-90f9-2eb340443535
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.github/workflows/ci.yamlbaml_language/.config/nextest.tomlbaml_language/Cargo.tomlbaml_language/crates/bridge_cffi/Cargo.tomlbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_cffi/src/wasm.rsbaml_language/sdk_tests/crates/typescript_web/.gitignorebaml_language/sdk_tests/crates/typescript_web/Cargo.tomlbaml_language/sdk_tests/crates/typescript_web/build.rsbaml_language/sdk_tests/crates/typescript_web/setup.ps1baml_language/sdk_tests/crates/typescript_web/setup.shbaml_language/sdk_tests/crates/typescript_web/src/lib.rsbaml_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/package_web.jsonbaml_language/sdk_tests/harness_setup/src/templates/tsconfig_web.jsonbaml_language/sdk_tests/harness_setup/src/templates/vitest_web.config.tsbaml_language/sdk_tests/harness_setup/src/typescript_web.rsbaml_language/sdks/web/bridge_web/.gitignorebaml_language/sdks/web/bridge_web/Cargo.toml
| Get-ChildItem -Directory | ForEach-Object { | ||
| $generated = Join-Path $_.FullName 'generated' | ||
| if (Test-Path $generated) { | ||
| Push-Location $generated | ||
| try { | ||
| pnpm install --force --ignore-workspace --ignore-scripts | ||
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } | ||
| pnpm update @boundaryml/baml-bridge-web --force --ignore-workspace --ignore-scripts | ||
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } | ||
| } finally { Pop-Location } | ||
| } | ||
| } | ||
|
|
||
| if ($env:NEXTEST_ENV) { Add-Content -Path $env:NEXTEST_ENV -Value 'SDK_TEST_TYPESCRIPT_WEB_SETUP=1' } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enumerate the generated fixtures, not the crate directory.
After Set-Location $PSScriptRoot, Get-ChildItem -Directory scans baml_language/sdk_tests/crates/typescript_web, but generated packages are written under baml_language/sdk_tests/harness_setup/<fixture>/generated. The loop therefore skips fixture setup and still writes SDK_TEST_TYPESCRIPT_WEB_SETUP=1.
Proposed fix
-Get-ChildItem -Directory | ForEach-Object {
+$HarnessSetupRoot = Join-Path $WorkspaceRoot 'sdk_tests\harness_setup'
+Get-ChildItem -Directory $HarnessSetupRoot | ForEach-Object {📝 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.
| Get-ChildItem -Directory | ForEach-Object { | |
| $generated = Join-Path $_.FullName 'generated' | |
| if (Test-Path $generated) { | |
| Push-Location $generated | |
| try { | |
| pnpm install --force --ignore-workspace --ignore-scripts | |
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } | |
| pnpm update @boundaryml/baml-bridge-web --force --ignore-workspace --ignore-scripts | |
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } | |
| } finally { Pop-Location } | |
| } | |
| } | |
| if ($env:NEXTEST_ENV) { Add-Content -Path $env:NEXTEST_ENV -Value 'SDK_TEST_TYPESCRIPT_WEB_SETUP=1' } | |
| $HarnessSetupRoot = Join-Path $WorkspaceRoot 'sdk_tests\harness_setup' | |
| Get-ChildItem -Directory $HarnessSetupRoot | ForEach-Object { | |
| $generated = Join-Path $_.FullName 'generated' | |
| if (Test-Path $generated) { | |
| Push-Location $generated | |
| try { | |
| pnpm install --force --ignore-workspace --ignore-scripts | |
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } | |
| pnpm update `@boundaryml/baml-bridge-web` --force --ignore-workspace --ignore-scripts | |
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } | |
| } finally { Pop-Location } | |
| } | |
| } | |
| if ($env:NEXTEST_ENV) { Add-Content -Path $env:NEXTEST_ENV -Value 'SDK_TEST_TYPESCRIPT_WEB_SETUP=1' } |
🤖 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/typescript_web/setup.ps1` around lines 16 -
29, Update the fixture enumeration in the setup script to scan the generated
packages under the harness_setup fixture root rather than the crate directory.
Preserve the existing pnpm install/update and exit handling for each generated
directory, and only write SDK_TEST_TYPESCRIPT_WEB_SETUP after those fixtures
have been processed.
| "test": "vitest run --config vitest.config.ts" | ||
| }, | ||
| "dependencies": { | ||
| "@boundaryml/baml-bridge-web": "file:../../../../../sdks/web/bridge_web" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'File location and nearby context:\n'
git ls-files 'baml_language/sdk_tests/harness_setup/src/templates/package_web.json' \
'baml_language/sdk_tests/harness_setup/*/generated/package_web.json' \
'baml_language/sdks/web/bridge_web/package.json' \
'sdks/web/bridge_web/package.json' || true
printf '\nTemplate file contents:\n'
cat -n baml_language/sdk_tests/harness_setup/src/templates/package_web.json
printf '\nBridge package path check:\n'
python3 - <<'PY'
from pathlib import Path
template = Path('baml_language/sdk_tests/harness_setup/src/templates/package_web.json')
print('template exists:', template.exists())
for p in [
Path('baml_language/sdk_tests/harness_setup/fixture/generated/package_web.json'),
Path('baml_language/sdk_tests/harness_setup/some_fixture/generated/package_web.json'),
]:
print(p, 'parent count', len(p.parents))
PYRepository: BoundaryML/baml
Length of output: 1213
Fix the bridge dependency path for generated fixtures.
baml_language/sdk_tests/harness_setup/<fixture>/generated/package_web.json:10 should point to baml_language/sdks/web/bridge_web; ../../../../../sdks/web/bridge_web walks past baml_language, so pnpm resolves the wrong location.
Proposed fix
- "`@boundaryml/baml-bridge-web`": "file:../../../../../sdks/web/bridge_web"
+ "`@boundaryml/baml-bridge-web`": "file:../../../../sdks/web/bridge_web"📝 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.
| "@boundaryml/baml-bridge-web": "file:../../../../../sdks/web/bridge_web" | |
| "`@boundaryml/baml-bridge-web`": "file:../../../../sdks/web/bridge_web" |
🤖 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/harness_setup/src/templates/package_web.json` at line
10, Update the `@boundaryml/baml-bridge-web` dependency path in package_web.json
to resolve to baml_language/sdks/web/bridge_web, correcting the relative path so
generated fixtures reference the intended bridge package.
Summary by CodeRabbit