Repository navigation
ci: Introduce ast-grep, rule skill guidance, extern c panic rule - #2595
colin-higgins wants to merge 3 commits into
Conversation
colin-higgins
changed the title
![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: 9a03908415
ℹ️ 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".
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 95c9eaf | Docs | View more details | Give us feedback! |
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
|
BenchmarksComparisonBenchmark execution time: 2026-09-29 21:03:35 Comparing candidate commit 95c9eaf in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 176 metrics, 0 unstable metrics.
|
ast-grep cannot expand macros, so a new c_setters! path produced an uncontained export with no matching function_item. Also propagate filtered scan failures and diff github.event.before on push so origin/main being HEAD does not empty the range. Co-authored-by: Cursor <cursoragent@cursor.com>
| If a correct rule would fail on a large, intentional legacy set, do | ||
| **not** rewrite the repo and do **not** fall back to regex. Copy the | ||
| FFI panic rule: |
There was a problem hiding this comment.
I disagree with this. If we decide a pattern is harmful and lint against it, we should lint the whole codebase.
It is fine to have escape hatches in the codebase, adding local lint skips in comments but we should not have code that causes lint violations go unnoticed until someone decides to touch it for whatever reason.
I think the pattern for adding lints should probably be stacked PR:
- One adding skips for parts of the code that would fails
- A stacked PR on top of this adding the lint in the CI
This ways it's easy to know what code is non compliant, and if the lint actually makes sense to apply to the whole codebase
| # Format / Clippy (touched crate) | ||
| cargo +nightly-2026-07-26 fmt --all -- --check | ||
| cargo +stable clippy -p <crate> --all-targets -- -D warnings | ||
|
|
||
| # Workspace dependency declarations | ||
| (cd .github/actions && cargo run -p workspace-deps-lint) | ||
|
|
||
| # Deps / licenses | ||
| cargo deny check | ||
| cargo machete --with-metadata --skip-target-dir | ||
|
|
||
| # Crypto graph | ||
| ./scripts/check_crypto_providers.sh |
There was a problem hiding this comment.
these should already be in AGENTS.md so prefer referring to it in the skill. This way we have only one place to maintain
| alt = "|".join(re.escape(name) for name in sorted(macro_names)) | ||
| rule = f""" | ||
| id: ffi-macro-invocation-emits-extern-c | ||
| language: rust | ||
| rule: | ||
| kind: macro_invocation | ||
| regex: "(^|::)({alt})!" |
There was a problem hiding this comment.
A bash script that contains inline python that contains inline ast-grep rules?? Seems a bit too.. sloppy honestly
| exit 0 | ||
| fi | ||
|
|
||
| DIFF="$(git diff -U0 --diff-filter=ACMR "$BASE" -- '*.rs' || true)" |
There was a problem hiding this comment.
This needs --no-ext-diff: with an external diff tool configured (difftastic etc.) no added lines get parsed, so the check silently passes locally.
| DIFF="$(git diff -U0 --diff-filter=ACMR "$BASE" -- '*.rs' || true)" | |
| DIFF="$(git --no-pager diff --no-ext-diff --no-color --no-textconv -U0 --diff-filter=ACMR "$BASE" -- '*.rs' || true)" |
| kind: visibility_modifier | ||
| regex: "^pub$" | ||
| - has: | ||
| kind: function_modifiers | ||
| has: | ||
| kind: extern_modifier | ||
| has: | ||
| kind: string_literal | ||
| regex: '^"C"$' |
There was a problem hiding this comment.
This only matches pub extern "C" fn, so it misses non-pub #[no_mangle] functions, extern fn without an ABI string, and extern "system".
| ignores: | ||
| - "**/tests/**" | ||
| - "**/examples/**" | ||
| - "symbolizer-ffi/**" |
There was a problem hiding this comment.
Could we add datadog-sidecar-ffi/** to ignores? AGENTS.md rules out catch_unwind there, so otherwise it'd need ~140 allow comments.
This branch has not been deployed
3 participants
What does this PR do?
Adds a blocking ast-grep guard that rejects newly introduced
pub extern "C"FFI entry points unless their bodies acknowledge panic containment. Matching is done on the Rust AST (not regex). Enforcement is diff-scoped so the ~400 existing accessors are left alone.Also lands the ast-grep project (pinned runner, Lint job, pre-commit) and an
add-lint-ruleskill so later review patterns follow the same path.Changes
ffi-extern-c-panic-containmentmatchespub/pub unsafeextern "C"function items and accepts:std::panic::catch_unwindwrap_with_ffi_result!/wrap_with_void_ffi_result!*_no_catch!variants (explicit acknowledgement)catch_panic!(data-pipeline-ffi and similar)// allow(ffi-panic-boundary): <justification>on a line directly above the function is accepted; empty justifications and names that appear only in comments are not.scripts/run-ffi-panic-lint.shintersects ast-grep hits withgit diff -U0added signature lines. Git decides what is new; ast-grep decides what matched../scripts/run-ast-grep.shis the one local/CI command: rule tests, whole-repo scan (this rule off so legacy hits stay quiet), then the diff-scoped FFI check. The Lint job uses the same script (fetch-depth: 0). Failures emit GitHub Actions annotations.add-lint-ruleskill (including “do not parse Rust with regex”).Motivation
Panics that cross C ABI boundaries can abort embedding runtimes. Clippy’s panic/unwrap lints only see explicit panic constructs, not panics from callees, so recent reviews kept flagging new uncontained FFI entry points (e.g. PRs #2545, #2551, #2566, #2569). A whole-repo error scan would fail on hundreds of intentional legacy accessors; a diff-scoped ast-grep rule blocks regressions without that rewrite.
This replaces the regex / ad-hoc brace-matcher approach in the earlier
ffi-panic-linthelper crate.Additional Notes
The check is additive and does not modify any existing FFI function or runtime behavior.
datadog-sidecar-ffistill usespanic=abort(do not addcatch_unwindthere); new sidecar entry points should use*_no_catch!or a justified allow.How to test the change?
Confirm an unchanged legacy accessor in a touched file does not fail.