fix: fail closed on event sanitizer errors#482
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
|
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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (13)**/*.rs📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Files:
**/*.{rs,py}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,js,mjs,cjs,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,go,js,ts,c,h}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,go,js,ts}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Files:
{crates/**/src/**/*.rs,python/**/*.py}📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Files:
**/*📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{rs,py,go,js,ts}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Files:
crates/{python,ffi,node}/**/*⚙️ CodeRabbit configuration file
Files:
crates/node/**/*.{js,mjs,cjs,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (3)
WalkthroughEvent sanitizer failure paths across Node.js, Python, FFI, and native plugin integrations now return default cleared fields instead of original input fields. Tests verify that invalid, failed, or unmarshalable sanitizer results clear observability data. ChangesEvent sanitizer failure handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Will Killian <wkillian@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/ffi/src/callable.rs`:
- Line 837: Update the sanitizer callback handling in the native plugin path so
deserialization or callback errors discard the original fields and return the
default redacted set. Replace the fallback that preserves fields with the
established fail-closed default behavior, while leaving successful sanitizer
results unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 15c30d2c-104a-4243-8c8f-e09c092eee2d
📒 Files selected for processing (3)
crates/ffi/src/callable.rscrates/ffi/tests/unit/callable_tests.rsgo/nemo_relay/event_sanitizers_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (20)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/ffi/tests/unit/callable_tests.rscrates/ffi/src/callable.rs
crates/ffi/**
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
Rebuild the FFI crate in release mode so the shared library and header stay in sync when making changes to crates/ffi
Files:
crates/ffi/tests/unit/callable_tests.rscrates/ffi/src/callable.rs
crates/ffi/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/ffi, also usetest-ffi-surfacefor validationUse C FFI export names prefixed with
nemo_relay_in the raw C FFI layer.
Files:
crates/ffi/tests/unit/callable_tests.rscrates/ffi/src/callable.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/ffi/tests/unit/callable_tests.rscrates/ffi/src/callable.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/ffi/tests/unit/callable_tests.rscrates/ffi/src/callable.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/ffi/tests/unit/callable_tests.rsgo/nemo_relay/event_sanitizers_test.gocrates/ffi/src/callable.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/ffi/tests/unit/callable_tests.rsgo/nemo_relay/event_sanitizers_test.gocrates/ffi/src/callable.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/ffi/tests/unit/callable_tests.rsgo/nemo_relay/event_sanitizers_test.gocrates/ffi/src/callable.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/ffi/tests/unit/callable_tests.rsgo/nemo_relay/event_sanitizers_test.gocrates/ffi/src/callable.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/ffi/tests/unit/callable_tests.rsgo/nemo_relay/event_sanitizers_test.gocrates/ffi/src/callable.rs
crates/{python,ffi,node}/**/*
⚙️ CodeRabbit configuration file
crates/{python,ffi,node}/**/*: Treat binding changes as public API changes. Check for parity with the other language bindings, FFI ownership/lifetime safety,
callback error propagation, stable type conversion, and consistent async/stream semantics.
Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere.
Files:
crates/ffi/tests/unit/callable_tests.rscrates/ffi/src/callable.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/ffi/tests/unit/callable_tests.rsgo/nemo_relay/event_sanitizers_test.go
go/nemo_relay/**/*.go
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
go/nemo_relay/**/*.go: Format changed Go packages withcd go/nemo_relay && go fmt ./...
Run Go tests withjust test-goto build and test the NeMo Relay Go binding
Usejust build-gowhen you want an explicit build-only pass or need the artifact for other work
Usejust ci=true test-gowhen you need the CI-style coverage and JUnit path
On macOS, setDYLD_LIBRARY_PATHto the../../target/releasedirectory before running the rawgo testcommand directlyUse
PascalCasefor public Go APIs.
Files:
go/nemo_relay/event_sanitizers_test.go
**/*.go
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When changing the experimental Go binding, format Go code with
gofmtand keepgo vet ./...passing.
Files:
go/nemo_relay/event_sanitizers_test.go
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
go/nemo_relay/event_sanitizers_test.go
{python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update language wrapper helpers such as Python wrapper modules, Python type stubs, and Go shorthand packages when the new behavior belongs in those helper layers.
Files:
go/nemo_relay/event_sanitizers_test.go
**/*.{py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Keep Python, Go, and Node.js config objects and subscriber/exporter methods aligned so all bindings expose the same logical knobs and semantics.
Files:
go/nemo_relay/event_sanitizers_test.go
go/nemo_relay/**
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Keep shared plugin helpers in
go/nemo_relayaligned with plugin registration, composition, and lifecycle behavior.
Files:
go/nemo_relay/event_sanitizers_test.go
go/nemo_relay/**/*
⚙️ CodeRabbit configuration file
go/nemo_relay/**/*: Review Go binding changes for cgo memory ownership, race safety, callback cleanup, idiomatic exported APIs, and parity with Rust/FFI behavior.
Any API change should include focused Go tests and consider race-test behavior.
Files:
go/nemo_relay/event_sanitizers_test.go
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/ffi/src/callable.rs
🔇 Additional comments (2)
crates/ffi/tests/unit/callable_tests.rs (1)
556-564: LGTM!go/nemo_relay/event_sanitizers_test.go (1)
41-53: 🔒 Security & PrivacyDrop the category-profile setup suggestion.
EmitEventingo/nemo_relayonly accepts parent, data, metadata, and timestamp options, so this test cannot seed a category profile through the Go API.> Likely an incorrect or invalid review comment.
Signed-off-by: Will Killian <wkillian@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/core/tests/integration/native_plugin_tests.rs`:
- Around line 772-776: Update the subscriber setup in the native event sanitizer
test to call NativePluginTestCleanup::mark_subscriber_registered after
register_subscriber succeeds, ensuring Drop can deregister it during unwinding.
Preserve the existing explicit cleanup behavior and subscriber registration
flow.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1d01a665-bdf2-4eb3-aeaa-9e86349db29e
📒 Files selected for processing (6)
crates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/ffi/tests/unit/api/registry_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (22)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/ffi/tests/unit/api/registry_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
crates/ffi/**
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
Rebuild the FFI crate in release mode so the shared library and header stay in sync when making changes to crates/ffi
Files:
crates/ffi/tests/unit/api/registry_tests.rs
crates/ffi/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/ffi, also usetest-ffi-surfacefor validationUse C FFI export names prefixed with
nemo_relay_in the raw C FFI layer.
Files:
crates/ffi/tests/unit/api/registry_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/ffi/tests/unit/api/registry_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/ffi/tests/unit/api/registry_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/ffi/tests/unit/api/registry_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/ffi/tests/unit/api/registry_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/ffi/tests/unit/api/registry_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/ffi/tests/unit/api/registry_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/ffi/tests/unit/api/registry_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
crates/{python,ffi,node}/**/*
⚙️ CodeRabbit configuration file
crates/{python,ffi,node}/**/*: Treat binding changes as public API changes. Check for parity with the other language bindings, FFI ownership/lifetime safety,
callback error propagation, stable type conversion, and consistent async/stream semantics.
Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere.
Files:
crates/ffi/tests/unit/api/registry_tests.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/ffi/tests/unit/api/registry_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
{crates/core,crates/adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation
Files:
crates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
{crates/core/src/plugin/dynamic/**,crates/plugin/**,crates/worker/**,crates/worker-proto/**,crates/types/**,python/plugin/**,examples/rust-native-plugin/**,examples/python-grpc-worker-plugin/**,docs/build-plugins/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Keep the stable boundary explicit: native plugins cross a C ABI, and worker plugins cross
grpc-v1.
Files:
crates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rs
{crates/core/src/plugin/dynamic/**/*.rs,examples/rust-native-plugin/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not pass Rust runtime types, trait objects, futures, or allocator-owned strings across the native dynamic-library boundary.
Files:
crates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
{crates/core/src/plugin/dynamic/**,examples/rust-native-plugin/**,examples/python-grpc-worker-plugin/**,docs/build-plugins/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Native and worker plugins are trusted extensions; document that native plugins are in-process and unsandboxed, and worker plugins provide process isolation but not a security sandbox.
Files:
crates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rs
{crates/core/src/plugin/dynamic/**/*.rs,crates/plugin/**/*.rs,crates/worker/**/*.rs,crates/worker-proto/**/*.rs,python/plugin/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Manifest validation must cover kind, compatibility, load contract, integrity, capability mismatch, and disabled-plugin behavior.
Files:
crates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rs
crates/core/src/plugin/dynamic/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
The native loader must keep libraries alive until registered callbacks are cleared and must deregister plugin kinds before unload.
Files:
crates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rs
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
crates/{core,adaptive}/**/*.rs
⚙️ CodeRabbit configuration file
crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/fixtures/native_plugin/src/lib.rs
🔇 Additional comments (2)
crates/core/src/plugin/dynamic/native.rs (1)
1500-1500: 📐 Maintainability & Code QualityVerify the required Rust, core, and FFI validation matrix before handoff.
crates/core/src/plugin/dynamic/native.rs#L1500-L1500: run the Rust checks plus core-runtime validation.crates/core/src/plugin/dynamic/worker.rs#L1063-L1068: run the Rust checks plus core-runtime validation.crates/core/tests/fixtures/native_plugin/src/lib.rs#L422-L437: run the Rust checks plus core-runtime validation.crates/core/tests/integration/native_plugin_tests.rs#L749-L797: run the Rust checks plus core-runtime validation.crates/core/tests/unit/dynamic_worker_tests.rs#L1044-L1045: run the Rust checks plus core-runtime validation.crates/ffi/tests/unit/api/registry_tests.rs#L338-L339: run FFI surface validation and rebuild the FFI crate in release mode.Please attach results for
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust,cargo deny check,validate-change, the full Rust/Python/Go/Node matrix,test-ffi-surface, anduv run pre-commit run --all-files. Also confirm dependent skills/docs were updated if they document sanitizer fallback semantics.As per coding guidelines, Rust changes require formatting, Clippy, and Rust tests;
crates/corerequires broader validation;crates/ffirequires FFI surface validation and release rebuilding; and handoff requires full pre-commit validation.Source: Coding guidelines
crates/core/tests/unit/dynamic_worker_tests.rs (1)
1044-1044: LGTM!Also applies to: 1119-1121, 1142-1145
…vent-sanitizer-fail-closed Signed-off-by: Will Killian <wkillian@nvidia.com> # Conflicts: # crates/node/src/api/mod.rs # crates/node/src/callable.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/node/tests/event_sanitizers_tests.mjs (1)
89-111: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAssert that all event fields are cleared.
These fail-closed tests only inspect
data. Include non-null metadata (and a category-profile fixture where applicable), then assert every emitted sanitizer field is cleared in the direct, thread-safe, and plugin-owned failure cases. Otherwise, a regression leaking unsanitized metadata can pass.As per path instructions, tests should cover the behavior promised by the changed API surface, including error paths.
Also applies to: 137-167, 167-187, 245-268
🤖 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 `@crates/node/tests/event_sanitizers_tests.mjs` around lines 89 - 111, Expand the fail-closed sanitizer tests around the invalid-results cases and the corresponding thread-safe and plugin-owned failure cases to provide non-null metadata, including a category-profile fixture where applicable. Assert that every emitted sanitizer field is cleared, not just events.at(-1).data, while preserving the existing callback-error assertions and coverage of each failure path.Source: Path instructions
crates/node/src/api/mod.rs (1)
2383-2386: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAlign both event-sanitizer documentation blocks with fail-closed behavior.
crates/node/src/api/mod.rs#L2383-L2386: update the global event-sanitizer docs to say callback, serialization, conversion, and invalid-result failures clear event fields and record the error.crates/node/src/api/mod.rs#L2864-L2867: apply the same fail-closed wording to scope-local event sanitizers.🤖 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 `@crates/node/src/api/mod.rs` around lines 2383 - 2386, Update the documentation blocks for global and scope-local event sanitizers in crates/node/src/api/mod.rs at lines 2383-2386 and 2864-2867 to consistently describe fail-closed behavior: callback, serialization, conversion, and invalid-result failures clear event fields and record the error for getLastCallbackError(). Apply the same wording to both sites; no implementation changes are needed.Source: Path instructions
🤖 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.
Outside diff comments:
In `@crates/node/src/api/mod.rs`:
- Around line 2383-2386: Update the documentation blocks for global and
scope-local event sanitizers in crates/node/src/api/mod.rs at lines 2383-2386
and 2864-2867 to consistently describe fail-closed behavior: callback,
serialization, conversion, and invalid-result failures clear event fields and
record the error for getLastCallbackError(). Apply the same wording to both
sites; no implementation changes are needed.
In `@crates/node/tests/event_sanitizers_tests.mjs`:
- Around line 89-111: Expand the fail-closed sanitizer tests around the
invalid-results cases and the corresponding thread-safe and plugin-owned failure
cases to provide non-null metadata, including a category-profile fixture where
applicable. Assert that every emitted sanitizer field is cleared, not just
events.at(-1).data, while preserving the existing callback-error assertions and
coverage of each failure path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 49c36f37-3400-4a1a-bf18-7022fa8263e0
📒 Files selected for processing (6)
crates/core/tests/integration/native_plugin_tests.rscrates/node/src/api/mod.rscrates/node/src/callable.rscrates/node/tests/event_sanitizers_tests.mjscrates/node/tests/scope_tests.mjscrates/node/tests/tools_tests.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (17)
crates/node/**/*.{js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
camelCasefor Node.js public APIs.
Files:
crates/node/tests/scope_tests.mjscrates/node/tests/tools_tests.mjscrates/node/tests/event_sanitizers_tests.mjs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/node/tests/scope_tests.mjscrates/node/tests/tools_tests.mjscrates/node/src/api/mod.rscrates/node/src/callable.rscrates/core/tests/integration/native_plugin_tests.rscrates/node/tests/event_sanitizers_tests.mjs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/node/tests/scope_tests.mjscrates/node/tests/tools_tests.mjscrates/node/src/api/mod.rscrates/node/src/callable.rscrates/core/tests/integration/native_plugin_tests.rscrates/node/tests/event_sanitizers_tests.mjs
crates/{python,ffi,node}/**/*
⚙️ CodeRabbit configuration file
crates/{python,ffi,node}/**/*: Treat binding changes as public API changes. Check for parity with the other language bindings, FFI ownership/lifetime safety,
callback error propagation, stable type conversion, and consistent async/stream semantics.
Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere.
Files:
crates/node/tests/scope_tests.mjscrates/node/tests/tools_tests.mjscrates/node/src/api/mod.rscrates/node/src/callable.rscrates/node/tests/event_sanitizers_tests.mjs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/node/tests/scope_tests.mjscrates/node/tests/tools_tests.mjscrates/core/tests/integration/native_plugin_tests.rscrates/node/tests/event_sanitizers_tests.mjs
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/node/src/api/mod.rscrates/node/src/callable.rscrates/core/tests/integration/native_plugin_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/node/src/api/mod.rscrates/node/src/callable.rscrates/core/tests/integration/native_plugin_tests.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/node/src/api/mod.rscrates/node/src/callable.rscrates/core/tests/integration/native_plugin_tests.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/node/src/api/mod.rscrates/node/src/callable.rscrates/core/tests/integration/native_plugin_tests.rs
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
crates/node/src/api/mod.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/node/src/api/mod.rscrates/node/src/callable.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/node/src/api/mod.rscrates/node/src/callable.rscrates/core/tests/integration/native_plugin_tests.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/node/src/api/mod.rscrates/node/src/callable.rscrates/core/tests/integration/native_plugin_tests.rs
{crates/core,crates/adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/tests/integration/native_plugin_tests.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation
Files:
crates/core/tests/integration/native_plugin_tests.rs
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/core/tests/integration/native_plugin_tests.rs
crates/{core,adaptive}/**/*.rs
⚙️ CodeRabbit configuration file
crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/core/tests/integration/native_plugin_tests.rs
🔇 Additional comments (9)
crates/core/tests/integration/native_plugin_tests.rs (1)
777-777: 📐 Maintainability & Code QualityRun the required Rust validation before handoff.
The cleanup fix is correctly placed. Because this is a Rust change, verify
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust, anduv run pre-commit run --all-fileswere run successfully.Source: Coding guidelines
crates/node/src/api/mod.rs (4)
1333-1390: 📐 Maintainability & Code QualityRun the required Rust and Node validation before handoff.
Run
just test-rust,cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, the Node test target covering the changed sanitizer tests, anduv run pre-commit run --all-files.As per coding guidelines, any Rust change must run the required Rust test, formatting, lint, and pre-commit commands before review or handoff.
Source: Coding guidelines
81-81: LGTM!Also applies to: 567-600, 721-725, 765-769, 805-811, 853-859, 899-905, 986-992, 1115-1121, 1273-1305, 1333-1390, 1526-1527
2434-2440: LGTM!Also applies to: 2456-2457, 2473-2474, 2491-2504, 2525-2533, 2548-2548, 2612-2628, 2643-2659, 2673-2687, 2707-2726
2925-2935: LGTM!Also applies to: 2953-2954, 2971-2972, 2987-3005, 3032-3043, 3061-3061, 3138-3157, 3178-3197, 3217-3235, 3260-3284
crates/node/src/callable.rs (2)
17-17: LGTM!Also applies to: 41-41, 88-95, 122-185, 213-238, 243-266, 270-292, 329-364, 392-426, 431-455, 459-481, 592-662
97-120: 🩺 Stability & AvailabilitySync middleware callbacks are sync-only
This wrapper is used only for guardrails/intercepts that are documented to accept synchronous callbacks. Promise-returning callbacks are handled by the separate
PromiseAwareFn/*_asyncAPIs, so this is not a bug on this path.> Likely an incorrect or invalid review comment.crates/node/tests/scope_tests.mjs (1)
336-339: LGTM!Also applies to: 341-343
crates/node/tests/tools_tests.mjs (1)
30-31: LGTM!Also applies to: 299-314, 526-564, 566-590, 678-700, 756-756
Signed-off-by: Will Killian <wkillian@nvidia.com>
|
/merge |
Overview
Fail closed when event sanitizer callbacks cannot produce valid sanitized fields, preventing unsanitized observability data from reaching exporters.
Details
Where should the reviewer start?
Start with
crates/core/src/plugin/dynamic/native.rsandcrates/core/src/plugin/dynamic/worker.rs, where callback failures now clear observable event fields; the FFI API test records the externally visible contract.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Bug Fixes
data/metadataand related fields) instead of preserving original values.Tests