fix: address runtime and tooling findings - #633
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:XL
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change extracts native async stream delivery into shared helpers and expands native plugin ABI tests for callbacks, continuations, scopes, codecs, registration, and stream adapters. It also corrects documentation and updates numeric and static-analysis annotations. ChangesNative plugin continuation and ABI
Documentation and static-analysis updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NativeEntry as native_async_next_invoke_stream
participant Delivery as deliver_native_async_next_stream
participant Continuation as LLM stream continuation
participant Callback as Native callback
NativeEntry->>Delivery: Delegate stream continuation
Delivery->>Continuation: Execute continuation
Continuation-->>Delivery: Return stream status and JSON chunk
Delivery->>Callback: Forward chunk or callback error
Delivery->>Callback: Signal completion
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/core/tests/unit/native_plugin_tests.rs (1)
3312-3378: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPass a real out pointer so the null-name branch runs.
native_scope_pushchecksout.is_null()first and returnsNullPointerfor the out slot. Lines 3362-3377 passptr::null_mut()forout, soread_name(null)never runs. The assertion passes for the wrong reason.Pass a valid out slot to reach the name validation, and assert the out slot stays null.
The null checks on lines 3336-3361 are unaffected.
♻️ Proposed fix
+ let mut out = ptr::null_mut(); assert_eq!( unsafe { native_scope_push( ptr::null(), NemoRelayNativeScopeType::Custom, ptr::null(), 0, ptr::null(), ptr::null(), ptr::null(), ptr::null(), - ptr::null_mut(), + &mut out, ) }, NemoRelayStatus::NullPointer ); + assert!(out.is_null()); }🤖 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/core/tests/unit/native_plugin_tests.rs` around lines 3312 - 3378, Update the native_scope_push call in assert_native_scope_stack_null_validation to pass a valid mutable out slot instead of ptr::null_mut(), allowing the null-name validation branch to execute. After the call, assert that the out slot remains null while preserving the expected NullPointer status; leave the other null checks unchanged.
🤖 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/unit/native_plugin_tests.rs`:
- Around line 367-427: Update assert_native_compatibility_edges so the
successful validate_relay_compatibility case derives its caret requirement from
env!("CARGO_PKG_VERSION") rather than hard-coding "^0.7". Keep the
version-independent negative assertions unchanged and continue asserting the
dynamically constructed requirement is accepted.
- Around line 4904-5024: Add allocation-reclamation checks in
crates/core/tests/unit/native_plugin_tests.rs at lines 4904-5024 by snapshotting
native_string_live_allocations() before each error callback and asserting the
count is unchanged afterward, covering the error wrapper callbacks shown. Also
update the stream test at lines 5592-5625 to make the same assertion for
ErrorWithJson and EndWithJson alongside drop_count; no other sites require
changes.
---
Outside diff comments:
In `@crates/core/tests/unit/native_plugin_tests.rs`:
- Around line 3312-3378: Update the native_scope_push call in
assert_native_scope_stack_null_validation to pass a valid mutable out slot
instead of ptr::null_mut(), allowing the null-name validation branch to execute.
After the call, assert that the out slot remains null while preserving the
expected NullPointer status; leave the other null checks 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: f4c5a80f-ff2f-4578-b4df-895c8f210f69
📒 Files selected for processing (5)
crates/core/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/native_plugin_tests.rscrates/node/adaptive.jspython/plugin/build_backend.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (27)
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/adaptive.js
crates/node/**/*.{js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
camelCasefor Node.js public APIs.
Files:
crates/node/adaptive.js
**/*.{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/adaptive.jspython/plugin/build_backend.pycrates/core/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/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/adaptive.jspython/plugin/build_backend.pycrates/core/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/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/adaptive.jscrates/core/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/native_plugin_tests.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/adaptive.jspython/plugin/build_backend.pycrates/core/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/native_plugin_tests.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.
**/*.{rs,py,go,js,ts}: When observability configuration or lifecycle is exposed, keep FFI and Python, Go, and Node.js binding-native config objects and subscriber/exporter methods aligned in logical knobs and semantics.
Require every OpenTelemetry endpoint to have a type and nonblank destination; resolveheader_envvalues at activation and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Preserve correct handling of mark events, start/end events, orphan cases, and span or trajectory fields derived from intended event data.
Run affected Rust tests andjust test-rustwhen event fields change; runjust test-python,just test-go, andjust test-nodewhen binding-native configuration or lifecycle changes.
Files:
crates/node/adaptive.jspython/plugin/build_backend.pycrates/core/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/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/adaptive.jspython/plugin/build_backend.pycrates/core/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/native_plugin_tests.rs
**/*.{md,mdx,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Examples and documentation must use each exporter's documented flush/deregister order before shutdown.
Files:
crates/node/adaptive.jspython/plugin/build_backend.py
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/adaptive.js
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
python/plugin/build_backend.pycrates/core/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/native_plugin_tests.rs
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: When changing the Python wrapper package, tests, or docs tooling, lint with Ruff (E,F,W,I), format with Ruff formatter (120-character lines, double quotes), and passtytype checking.
Add the SPDX license header to all Python source files using the#comment form.
Files:
python/plugin/build_backend.py
{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:
python/plugin/build_backend.pycrates/core/src/plugin/dynamic/native.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:
python/plugin/build_backend.pycrates/core/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.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:
python/plugin/build_backend.pycrates/core/src/plugin/dynamic/native.rs
{crates/plugin/**/*.rs,python/plugin/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Rust and Python SDKs must expose every supported registration surface.
Files:
python/plugin/build_backend.py
**/*.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/core/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/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/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/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/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/native_plugin_tests.rs
crates/core/src/{api/**/*.rs,api/runtime/**/*.rs,codec/**/*.rs,json.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Implement the new or changed public runtime behavior first in the Rust core, especially under
crates/core/src/api/and related core modules such ascrates/core/src/api/runtime/,crates/core/src/codec/, andcrates/core/src/json.rs.
Files:
crates/core/src/api/runtime/scope_stack.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/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/native_plugin_tests.rs
crates/core/src/api/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Preserve the documented pipeline order: conditional guardrails, request intercepts, request sanitization, execution intercepts, and response sanitization for tool and LLM execution; specialized sanitization, event creation, and dispatch for mark and scope events.
Files:
crates/core/src/api/runtime/scope_stack.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/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/native_plugin_tests.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.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.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.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/core/tests/unit/native_plugin_tests.rs
🧠 Learnings (2)
📚 Learning: 2026-07-28T03:31:05.964Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 564
File: crates/core/src/api/runtime/subscriber_dispatcher.rs:297-314
Timestamp: 2026-07-28T03:31:05.964Z
Learning: In this codebase’s runtime API, do not implement incremental native LLM stream forwarding via the native ABI v3 asynchronous middleware protocol (it can only settle a single JSON value via a one-shot completion handle and cannot forward stream chunks incrementally). If a latency-sensitive plugin needs streaming behavior, review for use of synchronous native stream intercepts or worker plugins instead of trying to chunk-deliver or incrementally forward over the ABI v3 async path.
Applied to files:
crates/core/src/api/runtime/scope_stack.rs
📚 Learning: 2026-07-28T20:07:29.880Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 571
File: crates/core/src/api/runtime/state.rs:996-1020
Timestamp: 2026-07-28T20:07:29.880Z
Learning: In NeMo Relay (RELAY-509), sanitizer callback failures must be treated as intentional fail-open behavior. When an event/tool (request/response) or LLM (request/response) sanitizer callback fails, the sanitizer chain should retain and publish the last valid event/payload snapshot (rather than dropping/invalidating the data) and log the failure including callback context (e.g., which sanitizer/callback failed and relevant identifiers). Apply this consistently across all sanitizer chains mentioned in the RELAY-509 documentation/migration guide.
Applied to files:
crates/core/src/api/runtime/scope_stack.rscrates/core/src/plugin/dynamic/native.rs
🔇 Additional comments (23)
crates/core/src/api/runtime/scope_stack.rs (2)
455-455: LGTM!
455-455: 📐 Maintainability & Code QualityConfirm the required formatter and test commands before handoff.
The PR summary confirms pre-commit checks and
just test-rust. It does not confirm all required commands:
cargo fmt --allcargo clippy --workspace --all-targets -- -D warningsjust test-rustjust test-pythonjust test-gojust test-nodeuv run pre-commit run --all-filesAlso confirm the Node-specific formatter, Ruff, and
tychecks for the other changed files.As per coding guidelines, Rust changes require strict formatting and Clippy checks, and changes under
crates/corerequire the full language matrix.Source: Coding guidelines
crates/node/adaptive.js (1)
160-160: LGTM!Also applies to: 171-171
python/plugin/build_backend.py (1)
163-164: 🔒 Security & PrivacyKeep the
# NOSONARsuppression._project_fileresolves the path and requires the candidate to be a direct child of_PROJECT_ROOT, which prevents an out-of-directory target.> Likely an incorrect or invalid review comment.crates/core/src/plugin/dynamic/native.rs (3)
2221-2225: LGTM!
2239-2258: LGTM!
2260-2310: LGTM!crates/core/tests/unit/native_plugin_tests.rs (16)
18-19: LGTM!Also applies to: 190-244
246-315: LGTM!
317-365: LGTM!
429-481: LGTM!
483-538: LGTM!
540-602: LGTM!
604-681: LGTM!Also applies to: 683-782, 784-826, 828-901
1059-1064: LGTM!Also applies to: 1066-1105, 1107-1142, 1144-1191, 1193-1214
3380-3412: LGTM!Also applies to: 3414-3436, 3438-3488, 3490-3580, 3582-3639, 3641-3657, 3659-3682
3918-3956: LGTM!Also applies to: 3958-4121, 4123-4262, 4264-4374, 4376-4408, 4410-4422
4424-4573: LGTM!
4751-4824: LGTM!Also applies to: 4877-4877
5120-5120: LGTM!Also applies to: 5135-5135, 5147-5192
5194-5311: LGTM!
5533-5542: LGTM!Also applies to: 5570-5579
5592-5594: LGTM!Also applies to: 5617-5625, 5691-5691, 5709-5722, 5758-5761, 5786-5789, 5837-5846
Signed-off-by: Will Killian <wkillian@nvidia.com>
1 participant
Overview
Addresses runtime and tooling findings separated from the broader coverage and complexity work for release/0.7.
Details
Validation performed on the equivalent combined tree:
Where should the reviewer start?
Start with crates/core/src/plugin/dynamic/native.rs and the corresponding cases in crates/core/tests/unit/native_plugin_tests.rs.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Bug Fixes
Documentation
Tests