Repository navigation
ci: enforce FFI panic containment - #2588
Open
colin-higgins wants to merge 1 commit into
Open
colin-higgins wants to merge 1 commit into
colin-higgins wants to merge 1 commit into
Conversation
Co-authored-by: colin-higgins <1801443+colin-higgins@users.noreply.github.com>
colin-higgins
requested review from
mabdinur,
pavlokhrebto and
sameerank
and removed request for
a team
September 29, 2026 14:34
github-actions
Bot
added
ci-build
telemetry
mini-agent
sidecar
common
data-pipeline
crashtracker
sampling
labels
Member
Author
|
Unsure what this bridge AR branch is... Closing while I investigate the agent choices of target branch. |
Contributor
🔒 Cargo Deny Results📦
|
Contributor
📚 Documentation Check Results📦
|
![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: c55667b2f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+17
to
+18
| "wrap_with_ffi_result_no_catch!", | ||
| "wrap_with_void_ffi_result_no_catch!", |
There was a problem hiding this comment.
When a newly added FFI entry point directly uses either _no_catch wrapper and its body panics, this lint accepts it even though those wrappers only convert returned errors and do not call catch_unwind (libdd-common-ffi/src/utils.rs). The panic can therefore escape the extern "C" boundary and abort the embedding process while the new CI guard reports success; reserve the markers for wrappers that actually catch unwinds, and require the documented justified waiver for genuinely panic-free entry points.
AGENTS.md reference: AGENTS.md:L93-L93
Useful? React with 👍 / 👎.
Comment on lines
+374
to
+375
| elif (( ${#v[@]} == 2 )); then | ||
| v=( "${v[0]}" "$(( 10#${v[1]} + 1 ))" 0 ) |
There was a problem hiding this comment.
> bounds before comparing floors
Cargo interprets >1.2 as greater than 1.2.0, so it still accepts 1.2.1; this branch instead converts it to a floor of 1.3.0. Consequently, changing a dependency requirement from >=1.2.1 to the equivalent >1.2 is incorrectly reported as a raised floor, causing the semver workflow to require a minor release (and potentially reject an otherwise valid patch-type PR). Normalize omitted components first and increment the patch component for exclusive bounds.
Useful? React with 👍 / 👎.
colin-higgins
changed the base branch from
BridgeAR/2026-09-08-thiserror-v2
to
main
September 29, 2026 14:45
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 3 Pipeline jobs failed
ℹ️ InfoNo other issues found (see more)🧪 All tests passed 🎯 Code Coverage (details) Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: c55667b | Docs | View more details | Give us feedback! |
Contributor
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
Collaborator
There was a problem hiding this comment.
I'm commenting on the approach more than on the actual code, but i feel like doing something like this with regex and ad-hoc rust syntax parsing is not the right tool.
I'd much rather see a small script (in rust, or not btw... could be python so we don't have to pay for build in the CI) that uses ast-grep rules to detect these patterns
BenchmarksComparisonBenchmark execution time: 2026-09-29 15:18:37 Comparing candidate commit c55667b in PR branch Found 16 performance improvements and 19 performance regressions! Performance is the same for 142 metrics, 0 unstable metrics.
|
Contributor
|
Fwiw: the sidecar-ffi does intentionally not do FFI panic containment. |
Contributor
FWIW, I think @iunanua is looking into having releases with artifacts for CI crates, so that you can download pre-built binaries. Put differently, "paying for build in the CI" is a temporary limitation, not an inherent property of having CI checks run as Rust executables. IMHO there's more value in having a uniform ecosystem for both the code and the CI, with a strongly typed and fast language like Rust, rather than scattering CI logic across bash, Rust, Python, etc. |
Member
Author
|
I opened another PR that introduces ast-grep, please check it out: #2595 |
This branch has not been 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
5 participants
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 does this PR do?
Adds a pull-request-only static-analysis guard that rejects newly introduced public C FFI entry points unless their bodies acknowledge panic containment.
Changes
pub extern "C" fnandpub unsafe extern "C" fnsignatures from zero-context Git diffs, including multiline signatures.catch_unwind, the common FFI wrapper macros, and the explicit_no_catchvariants.// allow(ffi-panic-boundary): <justification>escape hatch while rejecting empty justifications.Motivation
Panics that cross C ABI boundaries can abort embedding runtimes. Existing Clippy panic lints only detect explicit panic constructs and cannot catch panics originating in called library code, so recent review findings have repeatedly identified uncontained new FFI entry points. A diff-scoped guard prevents new regressions without imposing failures on intentional pre-existing accessors.
Additional Notes
The check is additive and does not modify any existing FFI function or runtime behavior. The
_no_catchwrappers count as an explicit acknowledgement that panic behavior was separately considered.How to test the change?
Run
cargo test -p ffi-panic-lintfrom.github/actions. The suite uses temporary Git repositories to prove that an uncontained new entry point fails, wrapped and justified entry points pass, unchanged legacy entry points are ignored, and an empty escape-hatch justification fails. Also runcargo clippy -p ffi-panic-lint --all-targets -- -D warningsandcargo fmt -p ffi-panic-lint -- --check.PR by Bits - View session in Datadog
Comment @DataDog to request changes