feat: expose event sanitizers in Node.js [4/6]#374
Conversation
github-actions
Bot
added
size:L
willkill07
changed the title
github-actions
Bot
added
size:XL
78c49b4 to
dc468b7
Compare
|
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 (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (15)crates/node/**/*.{js,ts,jsx,tsx,json}📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md)
Files:
crates/node/**/*.{ts,tsx,d.ts}📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md)
Files:
crates/node/**/*.{js,mjs,cjs,ts,tsx}📄 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:
**/*.{py,go,js,ts}📄 CodeRabbit inference engine (.agents/skills/maintain-observability/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:
**⚙️ CodeRabbit configuration file
Files:
crates/{python,ffi,node}/**/*⚙️ CodeRabbit configuration file
Files:
**/*.rs📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Files:
**/*.{rs,py}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{crates/**/src/**/*.rs,python/**/*.py}📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.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:
🧠 Learnings (1)📚 Learning: 2026-05-07T18:04:44.387ZApplied to files:
🔇 Additional comments (6)
WalkthroughAdds Node.js event sanitizer guardrail registration and JS/Rust callback bridging, updates PII redaction defaults to enable mark handling, and expands typings and tests for the new sanitizer flows. ChangesEvent sanitizer guardrails
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CoreRuntime
participant PersistentJsFunction
participant JSCallback
participant EventSanitizeFields
CoreRuntime->>PersistentJsFunction: call_event_sanitize(event, fields)
PersistentJsFunction->>JSCallback: invoke (event, fields)
JSCallback-->>PersistentJsFunction: sanitized fields JSON
PersistentJsFunction->>EventSanitizeFields: parse and convert result
PersistentJsFunction-->>CoreRuntime: EventSanitizeFn result
Possibly related PRs
Suggested labels: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:L
Signed-off-by: Will Killian <wkillian@nvidia.com>
dc468b7 to
53da5ed
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/node/src/api/mod.rs`:
- Around line 505-535: The new public Node.js sanitizer registration APIs
exposed through add_plugin_event_sanitizer and the
registerMarkSanitizeGuardrail/registerScopeSanitizeStartGuardrail/registerScopeSanitizeEndGuardrail
bindings need documentation/skills coverage tracked. Update the relevant
maintainer or consumer docs/skills plan for the Node.js API surface in this
area, and make sure the global and scope-local N-API exports are explicitly
accounted for, even if the actual docs landing is deferred to a follow-up PR.
- Around line 1094-1124: Add the missing SAFETY comments to each unsafe block in
call_event_sanitize, matching the style used in call_validate. Document why
napi_get_reference_value and the from_raw_unchecked conversions for JsFunction
and JsUnknown are safe here, especially around the borrowed reference lifetime
and the validity of the napi values before calling func.call and
EventSanitizeFields::from_napi_value.
- Around line 1094-1124: The `call_event_sanitize` helper currently accepts any
object-like return from the sanitizer, which lets Promise-like values or empty
objects slip through and wipe fields. Update `call_event_sanitize` in
`crates/node/src/api/mod.rs` to explicitly reject thenables/async returns before
deserializing, and also fail when the returned object has no recognized
`EventSanitizeFields` keys so `data`, `category_profile`, and `metadata` are not
cleared accidentally.
- Around line 1163-1177: The direct-path sanitizer wrapper in the
`Ok(Arc::new(move |event, fields| { ... }))` closure swallows
`event.try_to_json_value()` failures by falling back to `Json::Null`, then still
calls `direct.call_event_sanitize`, unlike
`callable.rs::wrap_js_event_sanitize_fn`. Update this path to record a callback
error when event serialization fails (matching the background-path message
style) and return the original `fields` immediately instead of invoking the
sanitizer callback with a null event. Keep the behavior aligned with the
background path so both `record_callback_error` and the callback short-circuit
happen consistently on serialization failure.
In `@crates/node/tests/event_sanitizers_tests.mjs`:
- Around line 89-103: The current test in event_sanitizers_tests.mjs only covers
a non-object sanitizer return, but it should also exercise the
valid-object/wrong-shape fail-open path from call_event_sanitize in
crates/node/src/api/mod.rs. Add coverage that registers a sanitizer returning an
empty object or thenable and asserts the expected behavior for that shape
validation case, including the error recording/fail-open outcome. Use the
existing test helpers and symbols like registerMarkSanitizeGuardrail, event,
flushSubscribers, and getLastCallbackError so the new assertion matches the
changed API behavior.
🪄 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: a0d87afc-c81c-4414-9c5c-a18fafeb001b
📒 Files selected for processing (9)
crates/node/pii_redaction.d.tscrates/node/pii_redaction.jscrates/node/plugin.d.tscrates/node/src/api/mod.rscrates/node/src/callable.rscrates/node/src/types/mod.rscrates/node/tests/adaptive_tests.mjscrates/node/tests/event_sanitizers_tests.mjscrates/node/tests/pii_redaction_tests.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (16)
crates/node/**/*.{js,ts,jsx,tsx,json}
📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md)
Format changed Node files with
npm run format --workspace=nemo-relay-node
Files:
crates/node/pii_redaction.d.tscrates/node/pii_redaction.jscrates/node/plugin.d.ts
crates/node/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md)
Use
npm run check:docstrings --workspace=nemo-relay-nodeto validate public API docstring checks when surface docs changed
Files:
crates/node/pii_redaction.d.tscrates/node/plugin.d.ts
crates/node/**/*.{js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
camelCasefor Node.js public APIs.
Files:
crates/node/pii_redaction.d.tscrates/node/pii_redaction.jscrates/node/tests/pii_redaction_tests.mjscrates/node/plugin.d.tscrates/node/tests/adaptive_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/pii_redaction.d.tscrates/node/pii_redaction.jscrates/node/tests/pii_redaction_tests.mjscrates/node/src/types/mod.rscrates/node/plugin.d.tscrates/node/tests/adaptive_tests.mjscrates/node/tests/event_sanitizers_tests.mjscrates/node/src/callable.rscrates/node/src/api/mod.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/pii_redaction.d.tscrates/node/pii_redaction.jscrates/node/src/types/mod.rscrates/node/plugin.d.tscrates/node/src/callable.rscrates/node/src/api/mod.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/pii_redaction.d.tscrates/node/pii_redaction.jscrates/node/src/types/mod.rscrates/node/plugin.d.tscrates/node/src/callable.rscrates/node/src/api/mod.rs
**/*.{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:
crates/node/pii_redaction.d.tscrates/node/pii_redaction.jscrates/node/plugin.d.ts
**/*
📄 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/pii_redaction.d.tscrates/node/pii_redaction.jscrates/node/tests/pii_redaction_tests.mjscrates/node/src/types/mod.rscrates/node/plugin.d.tscrates/node/tests/adaptive_tests.mjscrates/node/tests/event_sanitizers_tests.mjscrates/node/src/callable.rscrates/node/src/api/mod.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/pii_redaction.d.tscrates/node/pii_redaction.jscrates/node/src/types/mod.rscrates/node/plugin.d.tscrates/node/src/callable.rscrates/node/src/api/mod.rs
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
This file provides guidance to agents, including Claude Code and OpenAI Codex, when working in this repository.
Project Overview
NeMo Relay is a multi-language agent runtime framework for execution scopes, lifecycle events, middleware, plugins, and observability around tool and LLM calls. The core runtime is Rust. Primary supported bindings are Rust, Python, and Node.js. Go and the raw C FFI are experimental and source-first.
The shared runtime model is:
- Scope stacks decide where work belongs and which scope-local behavior is visible.
- Middleware registries decide what guardrails and intercepts run around managed calls.
- Plugins install reusable runtime behavior from configuration.
- Events record runtime behavior in ATOF form.
- Subscribers and exporters consume events in-process or export them to ATIF, OpenTelemetry, OpenInference, or other backends.
Repository Structure
The repository layout separates the Rust runtime, language bindings,
documentation, integrations, and agent-facing skills.crates/ core/ # Rust core runtime crate, published as nemo-relay adaptive/ # Adaptive runtime primitives and plugin components python/ # PyO3 native extension for the Python package ffi/ # Raw C ABI layer used by downstream bindings such as Go node/ # NAPI Node.js binding and JavaScript/TypeScript entry points python/ nemo_relay/ # Python wrapper package: scopes, tools, LLM, middleware, typed helpers, plugins, adaptive helpers tests/ # Python tests go/ nemo_relay/ # Experimental Go CGo binding and tests fern/ # Fern documentation site scripts/ # Stable wrappers and helper scripts; build/test/docs entry points live in justfile skills/ # Published Codex/agent skills for NeMo Relay usage patternsPrerequisites
Insta...
Files:
crates/node/pii_redaction.d.tscrates/node/pii_redaction.jscrates/node/tests/pii_redaction_tests.mjscrates/node/src/types/mod.rscrates/node/plugin.d.tscrates/node/tests/adaptive_tests.mjscrates/node/tests/event_sanitizers_tests.mjscrates/node/src/callable.rscrates/node/src/api/mod.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/node/pii_redaction.d.tscrates/node/pii_redaction.jscrates/node/tests/pii_redaction_tests.mjscrates/node/src/types/mod.rscrates/node/plugin.d.tscrates/node/tests/adaptive_tests.mjscrates/node/tests/event_sanitizers_tests.mjscrates/node/src/callable.rscrates/node/src/api/mod.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/node/tests/pii_redaction_tests.mjscrates/node/tests/adaptive_tests.mjscrates/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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/node/src/types/mod.rscrates/node/src/callable.rscrates/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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.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
🧠 Learnings (1)
📚 Learning: 2026-05-07T18:04:44.387Z
Learnt from: mnajafian-nv
Repo: NVIDIA/NeMo-Flow PR: 67
File: integrations/openclaw/src/modules.ts:1-2
Timestamp: 2026-05-07T18:04:44.387Z
Learning: In NVIDIA/NeMo-Flow, TypeScript source files should use `//` line comments for SPDX headers (e.g., `// SPDX-FileCopyrightText: ...` and `// SPDX-License-Identifier: ...`) rather than C-style block comments (`/* ... */`). The repo’s copyright checker enforces this mapping, so `//` SPDX headers in `.ts` files should not be flagged as a style violation.
Applied to files:
crates/node/pii_redaction.d.tscrates/node/plugin.d.ts
🔇 Additional comments (10)
crates/node/pii_redaction.d.ts (1)
33-46: LGTM!crates/node/pii_redaction.js (1)
15-26: LGTM!crates/node/tests/pii_redaction_tests.mjs (1)
21-21: LGTM!Also applies to: 48-51
crates/node/src/types/mod.rs (1)
12-12: LGTM!Also applies to: 302-313
crates/node/plugin.d.ts (1)
4-4: LGTM!Also applies to: 73-90
crates/node/src/callable.rs (2)
18-29: LGTM!Also applies to: 39-39
556-577: 🗄️ Data Integrity & IntegrationEmpty
{}results are valid here.EventSanitizeFieldsis entirely optional, andapply_sanitize_fields()writesNoneback into the event, so an empty object intentionally clears the observability fields rather than being malformed.> Likely an incorrect or invalid review comment.crates/node/src/api/mod.rs (1)
33-35: LGTM!Also applies to: 70-75, 428-461, 505-535, 2157-2198, 2608-2659
crates/node/tests/adaptive_tests.mjs (1)
116-118: LGTM!Also applies to: 160-162
crates/node/tests/event_sanitizers_tests.mjs (1)
1-181: LGTM!
|
P1 — #374: invalid object-shaped JS results do not fail open. |
bbednarski9
left a comment
bbednarski9
left a comment
There was a problem hiding this comment.
Just a single P1 above, otherwise pattern matches python and looks good
Signed-off-by: Will Killian <wkillian@nvidia.com>
coderabbitai
Bot
added
the
DO NOT MERGE
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/src/api/mod.rs (1)
1180-1187: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSame error-detail loss as the threadsafe path.
direct.call_event_sanitize(...).ok()discards thenapi::Errorreason fromevent_sanitize_fields_from_jsbefore logging the generic"JS event sanitizer callback failed"message. Same observability gap as incallable.rs::wrap_js_event_sanitize_fn— consider surfacing the underlying reason.♻️ Surface the underlying error
- let sanitized = direct - .call_event_sanitize(event_json, js_event_fields(&fields)) - .ok() - .and_then(core_event_fields); - if sanitized.is_none() { - record_callback_error("nemo_relay: JS event sanitizer callback failed".to_string()); - } - sanitized.unwrap_or(fields) + let sanitized = match direct.call_event_sanitize(event_json, js_event_fields(&fields)) { + Ok(result) => core_event_fields(result), + Err(error) => { + record_callback_error(format!( + "nemo_relay: JS event sanitizer callback failed: {error}" + )); + None + } + }; + sanitized.unwrap_or(fields)🤖 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 1180 - 1187, The JS event sanitizer path in `call_event_sanitize` is dropping the underlying `napi::Error` by converting the result with `.ok()` before logging. Update this flow so the error from `direct.call_event_sanitize(...)` is preserved and included when `record_callback_error` is called, similar to the fix needed in `callable.rs::wrap_js_event_sanitize_fn`. Keep the existing fallback to `fields`, but make the failure log carry the actual sanitizer failure reason for better diagnostics.crates/node/src/callable.rs (1)
546-580: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFallback path discards the underlying validation error.
event_sanitize_fields_from_jsreturns a descriptivenapi::Error(e.g. "must be an object", "non-thenable object", "at least one sanitizer field"), but.and_then(|result| result.ok())throws that reason away before the generic"invalid JS event sanitizer result"message is logged. This makes it hard to tell in production logs why a given sanitizer callback misbehaved (bad shape vs. thenable vs. empty object).♻️ Include the parse error in the logged message
- rx.recv() - .map_err(|error| { - record_callback_error(format!( - "nemo_relay: JS event sanitizer callback failed: {error}" - )); - }) - .ok() - .and_then(|result| result.ok()) - .and_then(|result| { + let parsed = rx + .recv() + .map_err(|error| { + record_callback_error(format!( + "nemo_relay: JS event sanitizer callback failed: {error}" + )); + }) + .ok() + .and_then(|result| match result { + Ok(fields) => Some(fields), + Err(error) => { + record_callback_error(format!( + "nemo_relay: invalid JS event sanitizer result: {error}" + )); + None + } + }); + parsed + .and_then(|result| { let category_profile = result .category_profile .map(serde_json::from_value) .transpose() .ok()?; Some(CoreEventSanitizeFields { data: result.data, category_profile, metadata: result.metadata, }) }) - .unwrap_or_else(|| { - record_callback_error("nemo_relay: invalid JS event sanitizer result".to_string()); - fields.clone() - }) + .unwrap_or_else(|| fields.clone())🤖 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/callable.rs` around lines 546 - 580, The fallback path in `event_sanitize_fields` / the `rx.recv()` result handling is dropping the underlying `napi::Error` reason before logging the generic invalid-result message. Preserve the error from `event_sanitize_fields_from_js` by carrying it through the `.and_then(|result| result.ok())` chain or by matching on the nested result, then include that specific validation failure in the `record_callback_error` message instead of only logging `"invalid JS event sanitizer result"`.
🤖 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 1180-1187: The JS event sanitizer path in `call_event_sanitize` is
dropping the underlying `napi::Error` by converting the result with `.ok()`
before logging. Update this flow so the error from
`direct.call_event_sanitize(...)` is preserved and included when
`record_callback_error` is called, similar to the fix needed in
`callable.rs::wrap_js_event_sanitize_fn`. Keep the existing fallback to
`fields`, but make the failure log carry the actual sanitizer failure reason for
better diagnostics.
In `@crates/node/src/callable.rs`:
- Around line 546-580: The fallback path in `event_sanitize_fields` / the
`rx.recv()` result handling is dropping the underlying `napi::Error` reason
before logging the generic invalid-result message. Preserve the error from
`event_sanitize_fields_from_js` by carrying it through the `.and_then(|result|
result.ok())` chain or by matching on the nested result, then include that
specific validation failure in the `record_callback_error` message instead of
only logging `"invalid JS event sanitizer result"`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 441060dd-2af9-4de5-812f-3a0e8e1d5632
📒 Files selected for processing (4)
crates/node/src/api/mod.rscrates/node/src/callable.rscrates/node/src/types/mod.rscrates/node/tests/event_sanitizers_tests.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
**/*.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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/node/src/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.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/node/src/types/mod.rscrates/node/tests/event_sanitizers_tests.mjscrates/node/src/callable.rscrates/node/src/api/mod.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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.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/types/mod.rscrates/node/src/callable.rscrates/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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.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/node/src/types/mod.rscrates/node/tests/event_sanitizers_tests.mjscrates/node/src/callable.rscrates/node/src/api/mod.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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.rs
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
This file provides guidance to agents, including Claude Code and OpenAI Codex, when working in this repository.
Project Overview
NeMo Relay is a multi-language agent runtime framework for execution scopes, lifecycle events, middleware, plugins, and observability around tool and LLM calls. The core runtime is Rust. Primary supported bindings are Rust, Python, and Node.js. Go and the raw C FFI are experimental and source-first.
The shared runtime model is:
- Scope stacks decide where work belongs and which scope-local behavior is visible.
- Middleware registries decide what guardrails and intercepts run around managed calls.
- Plugins install reusable runtime behavior from configuration.
- Events record runtime behavior in ATOF form.
- Subscribers and exporters consume events in-process or export them to ATIF, OpenTelemetry, OpenInference, or other backends.
Repository Structure
The repository layout separates the Rust runtime, language bindings,
documentation, integrations, and agent-facing skills.crates/ core/ # Rust core runtime crate, published as nemo-relay adaptive/ # Adaptive runtime primitives and plugin components python/ # PyO3 native extension for the Python package ffi/ # Raw C ABI layer used by downstream bindings such as Go node/ # NAPI Node.js binding and JavaScript/TypeScript entry points python/ nemo_relay/ # Python wrapper package: scopes, tools, LLM, middleware, typed helpers, plugins, adaptive helpers tests/ # Python tests go/ nemo_relay/ # Experimental Go CGo binding and tests fern/ # Fern documentation site scripts/ # Stable wrappers and helper scripts; build/test/docs entry points live in justfile skills/ # Published Codex/agent skills for NeMo Relay usage patternsPrerequisites
Insta...
Files:
crates/node/src/types/mod.rscrates/node/tests/event_sanitizers_tests.mjscrates/node/src/callable.rscrates/node/src/api/mod.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/node/src/types/mod.rscrates/node/tests/event_sanitizers_tests.mjscrates/node/src/callable.rscrates/node/src/api/mod.rs
crates/node/**/*.{js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
camelCasefor Node.js public APIs.
Files:
crates/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/event_sanitizers_tests.mjs
{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
🔇 Additional comments (3)
crates/node/src/api/mod.rs (1)
1095-1130: LGTM!Also applies to: 1171-1179
crates/node/tests/event_sanitizers_tests.mjs (1)
89-115: LGTM!Also applies to: 137-166
crates/node/src/types/mod.rs (1)
313-350: 🎯 Functional CorrectnessNo issue here —
has_own_propertykeeps missing fields out, andobject.get(name)preserves an explicitnullasSome(Json::Null), so{ data: null }still clears the field while absence stays unchanged.> Likely an incorrect or invalid review comment.
|
/merge |
#### Overview Documents the event sanitizer and PII redaction behavior implemented by the merged RELAY-409 stack (#371–#375). This docs-only PR is the final part of that six-PR stack. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Adds a dedicated event sanitizer reference covering callback replacement semantics, ordering, registration lifetimes, cross-language APIs, and dynamic-plugin protocol values. - Explains which mark and scope event fields sanitizers can replace and which lifecycle and identity fields remain immutable. - Documents PII `mark = true` defaults, opt-out behavior, generic scope `input` and `output` coverage, and subscriber/exporter privacy guarantees. - Updates related middleware, event, scope/mark, plugin-authoring, Python, Node.js, Go, and observability-skill guidance. - Contains no implementation changes. - Breaking changes: none. Validation: - `just docs` - `just docs-linkcheck` - `uv run pre-commit run --all-files` #### Where should the reviewer start? Start with `docs/reference/event-sanitizers.mdx`, then review `docs/configure-plugins/pii-redaction/configuration.mdx` for the privacy contract. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to: RELAY-409 - Relates to: #371, #372, #373, #374, and #375 ## Summary by CodeRabbit - **New Features** - Added observability sanitization for **mark** and **scope** events (including scope-start and scope-end) before delivery to subscribers/exporters. - Sanitizers can rewrite observability fields (`data`, `category_profile`, `metadata`) while keeping core identity/context fields immutable. - Expanded privacy redaction support with a configurable `mark` option (default enabled) for mark and generic scope events. - **Documentation** - Added a new Event Sanitizers reference covering registration levels, ordering, callback contracts, and examples. - Updated middleware and language-binding guidance plus Node.js/Go/Python overviews to reflect mark/scope sanitizer capabilities. Authors: - Will Killian (https://github.com/willkill07) Approvers: - https://github.com/lvojtku - Bryan Bednarski (https://github.com/bbednarski9) URL: #376
2 participants
Overview
Exposes the mark and scope event sanitizer registries from PRs #371 and #372 through the Node.js binding and plugin context.
Details
PluginContextand TypeScriptEventSanitizeFieldsdeclarations.markto PII configuration typing and defaults.Validation:
just test-node(251 tests), focused event sanitizer tests, 100% JavaScript wrapper coverage, formatting, docstring checks, Clippy, and pre-commit.Where should the reviewer start?
Start with
crates/node/src/api/mod.rs,crates/node/src/callable.rs, andcrates/node/tests/event_sanitizers_tests.mjs.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
marksupport to the default PII redaction configuration.