Repository navigation
Harden bridge_cffi release artifact hygiene - #4174
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds native CFFI artifact hygiene checks and compiler path remapping, wires bridge release builds into CI, and requires production releases to attest the originating canary CI run with scoped permissions and stricter job gates. ChangesCFFI release integrity
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant BridgeBuild
participant Hygiene
participant Release
CI->>BridgeBuild: run bridge CFFI release
BridgeBuild->>Hygiene: verify native artifact
Hygiene-->>BridgeBuild: return verification status
CI->>Release: dispatch with source SHA and CI run id
Release->>CI: attest canary CI run
CI-->>Release: return successful matching run
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 |
⏭️ 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 |
![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: 3
🧹 Nitpick comments (1)
baml_language/Cross.toml (1)
12-22: 📐 Maintainability & Code Quality | 🔵 TrivialConsider a single global passthrough instead of repeating it per target.
cross-rs supports
[build.env] passthrough = [...]applying to all targets; since all four Linux targets set the identical["RUSTFLAGS", "CFLAGS", "CXXFLAGS"]list, this could be consolidated into one global entry, unless other per-target env settings not shown in this excerpt make target-specific sections a hard requirement.♻️ Possible consolidation
+[build.env] +passthrough = ["RUSTFLAGS", "CFLAGS", "CXXFLAGS"] + [target.x86_64-unknown-linux-gnu.env] -passthrough = ["RUSTFLAGS", "CFLAGS", "CXXFLAGS"] - [target.aarch64-unknown-linux-gnu.env] -passthrough = ["RUSTFLAGS", "CFLAGS", "CXXFLAGS"] - [target.x86_64-unknown-linux-musl.env] -passthrough = ["RUSTFLAGS", "CFLAGS", "CXXFLAGS"] - [target.aarch64-unknown-linux-musl.env] -passthrough = ["RUSTFLAGS", "CFLAGS", "CXXFLAGS"]🤖 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/Cross.toml` around lines 12 - 22, Consolidate the identical passthrough environment configuration from the four target-specific sections into a single [build.env] passthrough entry containing RUSTFLAGS, CFLAGS, and CXXFLAGS, unless unseen target-specific settings require retaining those sections.
🤖 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/build2-bridge-cffi.reusable.yaml:
- Around line 197-206: Update the native_path_maps value used by CFLAGS and
CXXFLAGS to include /experimental:deterministic alongside the existing /pathmap
options, ensuring cl.exe applies the path mappings during Windows builds. Leave
the Rust RUSTFLAGS configuration unchanged.
In @.github/workflows/release-baml-language.yml:
- Around line 95-163: Update the retry loop in the “Attest production source CI
run” step so failures from the gh api call are handled without terminating under
set -euo pipefail. Guard the API request, log the transient error, and continue
through the existing attempt-based sleep/retry path, while preserving the
current validation and hard failures for successfully retrieved invalid or
unsuccessful run data.
In `@scripts/baml-bridge-cffi-hygiene`:
- Around line 36-48: Update the four path regex patterns in the anchor-only
checks to allow double and single quotes as valid preceding boundaries by adding
them to each negative lookbehind, while preserving the existing path
continuation classes and case-insensitive matching.
---
Nitpick comments:
In `@baml_language/Cross.toml`:
- Around line 12-22: Consolidate the identical passthrough environment
configuration from the four target-specific sections into a single [build.env]
passthrough entry containing RUSTFLAGS, CFLAGS, and CXXFLAGS, unless unseen
target-specific settings require retaining those sections.
🪄 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 Plus
Run ID: 88277fde-92cd-4cbb-9c8f-13adb9c8a774
📒 Files selected for processing (7)
.github/workflows/build2-bridge-cffi.reusable.yaml.github/workflows/ci.yaml.github/workflows/release-baml-language.ymlbaml_language/Cross.tomlbaml_language/sdks/csharp/bridge_csharp/tools/pack-product.shscripts/baml-bridge-cffi-hygienescripts/tests/test_release_pipeline_contract.py
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.
What changed
bridge_cffirelease targetCFLAGSandCXXFLAGSthrough all Linuxcrosscontainers/pathmapRoot cause
The producer applied
--remap-path-prefixonly to Rust. AWS-LC is compiled through CMake/C, so GNU/Linux artifacts retained/cargo/...source paths. Windows also compared Git Bash paths with compiler-emitted drive-letter paths, leavingC:\Users\runneradmin\.cargo\...strings. The defect was only detected later by the C# product-package gate because CI used fixture natives instead of the actual release matrix.Impact
Polluted native binaries now fail in their own producer matrix leg before upload. The same exact release matrix is required before merge and before canary release dispatch, while C# retains defense-in-depth over packaged bytes. Production releases are also bound to a successful
CI - BAML Languagecanary push run for the exact source commit.Validation
scripts/baml-language-version checkx86_64-unknown-linux-gnurelease-bridge-cffibuild passes the new checkerSummary by CodeRabbit
Security & Reliability
CI & Release
Packaging
Tests