refactor: address Sonar findings and raise Rust coverage - #632
refactor: address Sonar findings and raise Rust coverage#632willkill07 wants to merge 8 commits into
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:XXL
|
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:
WalkthroughThe PR enforces Clippy cognitive-complexity limits, extracts runtime logic into focused helpers, and expands coverage across adaptive caching, observability, plugins, FFI, Python, Go, Node, and CLI code. ChangesComplexity cleanup and coverage expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@Cargo.toml`:
- Around line 48-50: Update the [workspace.lints.clippy] configuration by
removing the deprecated cognitive_complexity lint and replacing it with a
supported equivalent such as excessive_nesting or too_many_lines, preserving the
existing deny-level enforcement.
In `@crates/core/src/plugin/dynamic/native.rs`:
- Around line 2264-2299: Update the native stream forwarding loop around
native_string_from_json so a serialization failure calls callback_guard.fail
with an appropriate error message instead of breaking and reporting successful
completion. Preserve the existing success terminal callback only for streams
that finish without serialization or stream errors.
In `@crates/ffi/tests/integration/api_tests.rs`:
- Around line 48-57: Add #[track_caller] to assert_native_status in both
crates/ffi/tests/integration/api_tests.rs (lines 48-57) and
crates/ffi/tests/unit/api_tests.rs (lines 49-58), preserving the existing
assert_status! call sites and assertion behavior while reporting failures at the
original test caller.
In `@crates/ffi/tests/unit/api/registry_tests.rs`:
- Around line 91-101: Update the expectation message in otlp_content_length to
explicitly mention that the request may be using transfer-encoding: chunked when
content-length is absent, while preserving the existing parsing behavior.
In `@crates/python/src/py_callable.rs`:
- Around line 1771-1791: Update call_event_sanitizer to match directly on
loop_affine rather than !loop_affine, and reorder the dispatch arms so each
branch condition clearly corresponds to the flag’s actual value while preserving
existing callback invocation behavior. Run cargo fmt --all, cargo clippy
--workspace --all-targets -- -D warnings, and just test-rust.
In `@crates/python/tests/coverage/py_types_coverage_tests.rs`:
- Around line 1984-1998: Update the assertions in the loop over the *_is_none
keys to require each decoded value equals Some(true), matching the adjacent
loop’s assert_eq! pattern. Keep the existing key list and error labels unchanged
so regressions where a field is no longer None fail the test.
In `@go/nemo_relay/test_helpers_test.go`:
- Around line 11-29: Add a doc comment immediately above
runTestInIsolatedWorkingDirectory stating that callers must not use it with
t.Parallel() because os.Chdir changes the process-wide working directory.
🪄 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: 4d029620-b0af-400b-a317-23b79703b8ea
📒 Files selected for processing (77)
Cargo.tomlclippy.tomlcrates/adaptive/src/plugin_component.rscrates/adaptive/src/response_cache/key.rscrates/adaptive/src/response_cache/replay.rscrates/adaptive/tests/unit/plugin_component_tests.rscrates/adaptive/tests/unit/response_cache/intercept_tests.rscrates/adaptive/tests/unit/response_cache/store_tests.rscrates/adaptive/tests/unit/trie/builder_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/plugins/mod.rscrates/cli/src/process/launcher.rscrates/cli/src/server/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/core/src/api/runtime/scope_stack.rscrates/core/src/logging/rotation.rscrates/core/src/observability/otel_genai.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin.rscrates/core/src/plugin/dynamic/native.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/src/stream.rscrates/core/tests/coverage/logging_rotation_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/atif_tests.rscrates/core/tests/unit/codec/anthropic_tests.rscrates/core/tests/unit/codec/openai_chat_tests.rscrates/core/tests/unit/codec/openai_responses_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/tests/unit/observability/atof_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/subscriber_dispatcher_tests.rscrates/core/tests/unit/types_tests.rscrates/ffi/tests/integration/api/coverage_sweeps_tests.rscrates/ffi/tests/integration/api_tests.rscrates/ffi/tests/integration/plugin_activation_tests.rscrates/ffi/tests/unit/api/core_tests.rscrates/ffi/tests/unit/api/coverage_sweeps_tests.rscrates/ffi/tests/unit/api/execution_tests.rscrates/ffi/tests/unit/api/plugin_tests.rscrates/ffi/tests/unit/api/registry_tests.rscrates/ffi/tests/unit/api_tests.rscrates/ffi/tests/unit/callable_tests.rscrates/ffi/tests/unit/types_tests.rscrates/node/adaptive.jscrates/node/tests/llm_tests.mjscrates/node/tests/tools_tests.mjscrates/node/tests/typed_tests.mjscrates/plugin/tests/typed_callbacks.rscrates/python/src/py_callable.rscrates/python/tests/coverage/py_adaptive_coverage_tests.rscrates/python/tests/coverage/py_api_coverage_tests.rscrates/python/tests/coverage/py_plugin_coverage_tests.rscrates/python/tests/coverage/py_types_coverage_tests.rsgo/nemo_relay/adaptive_plugin_test.gogo/nemo_relay/adaptive_runtime_test.gogo/nemo_relay/adaptive_test.gogo/nemo_relay/callbacks_test.gogo/nemo_relay/context_test.gogo/nemo_relay/coverage_gap_test.gogo/nemo_relay/llm_test.gogo/nemo_relay/nemo_relay.gogo/nemo_relay/otel_test.gogo/nemo_relay/plugin_activation_test.gogo/nemo_relay/test_helpers_test.gopython/plugin/build_backend.pyscripts/package_node_musllinux.mjs
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
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/core/tests/coverage/logging_sink_tests.rs`:
- Around line 123-127: Update the queue-capacity test around file_sink and
build_logger_error to assert the specific validation message for each capacity:
`must be greater than 0` when capacity is 0 and `exceeds maximum` when it is
MAX_FILE_SINK_QUEUE_ENTRIES + 1. Replace the non-empty check while preserving
coverage of both invalid inputs.
In `@crates/core/tests/unit/dynamic_worker_tests.rs`:
- Around line 308-342: Add a success-case assertion in the existing
optional_json_from_invoke_response test for an InvokeResponse whose result is
InvokeResult::Empty, verifying it returns Ok(None). Keep the current failure
assertions unchanged and place the new coverage alongside them.
- Around line 127-157: Strengthen
python_worker_process_launch_uses_the_managed_interpreter_and_endpoint_file by
making the stub managed interpreter record its invocation and environment to a
temporary file without relying on $1, since the shell receives bootstrap
arguments positionally. After spawn_worker_process completes, assert the record
confirms the managed interpreter was used and NEMO_RELAY_WORKER_ENDPOINT_FILE
contains the expected endpoint_file path, while preserving the existing
successful-exit assertion.
In `@crates/core/tests/unit/native_plugin_tests.rs`:
- Around line 4391-4406: Strengthen the assertion in the async middleware
registration test by retaining the NemoRelayStatus::InvalidArg check and adding
assert_last_error_contains with the loader’s incompatible relay_compat contract
error substring. Use the existing helper pattern from nearby tests and keep the
cleanup unchanged.
In `@crates/core/tests/unit/observability/plugin_component_tests.rs`:
- Around line 651-703: Guard both tests’ unsafe environment-variable mutations
with the existing crate::observability::test_mutex() lock. In
crates/core/tests/unit/observability/plugin_component_tests.rs lines 651-703,
acquire the guard at the start of
atof_stream_header_validation_reports_invalid_values_and_environment_names; do
the same at lines 3391-3424 for
http_upload_config_rejects_endpoint_timeout_and_header_errors, and update that
test’s SAFETY comment to accurately describe the mutex protection.
🪄 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: 7b5b9f02-6fc2-4879-bd27-30ad07bf7705
📒 Files selected for processing (9)
crates/core/src/plugin/dynamic/native.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_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 (19)
**/*.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/tests/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.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/tests/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.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/tests/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/native_plugin_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/tests/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/native_plugin_tests.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/core/tests/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.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/core/tests/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.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/core/tests/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.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/core/tests/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/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/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.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/core/tests/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.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/core/tests/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/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/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/native_plugin_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/core/tests/unit/plugin_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/native_plugin_tests.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.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/**/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.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,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.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
🧠 Learnings (1)
📚 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/plugin/dynamic/native.rs
🔇 Additional comments (21)
crates/core/tests/unit/llm_api_tests.rs (1)
149-158: LGTM!Also applies to: 187-200
crates/core/tests/coverage/logging_sink_tests.rs (1)
5-13: LGTM!Also applies to: 87-99, 101-122, 128-129, 131-148
crates/core/tests/unit/native_plugin_tests.rs (11)
540-602: LGTM!
3312-3314: LGTM!
3362-3377: LGTM!Also applies to: 3405-3409
3954-3955: LGTM!Also applies to: 4095-4118
4264-4374: LGTM!
4524-4571: LGTM!
4903-5014: LGTM!
5111-5111: LGTM!Also applies to: 5126-5126, 5138-5183
5185-5302: LGTM!
5524-5533: LGTM!Also applies to: 5561-5570, 5583-5585, 5608-5616, 5682-5682, 5700-5713, 5748-5751, 5776-5779, 5828-5837
3550-3579: 🩺 Stability & AvailabilityKeep
native_string_freefor this allocation. It reconstructsNativeHostStringbefore dropping it, so the deallocator matches the allocation.> Likely an incorrect or invalid review comment.crates/core/tests/unit/dynamic_worker_tests.rs (3)
6-8: LGTM!
195-206: LGTM!
344-367: LGTM!Also applies to: 994-1015
crates/core/src/plugin/dynamic/native.rs (1)
2260-2275: LGTM!crates/core/tests/unit/observability/plugin_component_tests.rs (1)
86-143: LGTM!Also applies to: 637-649, 705-723, 3426-3476
crates/core/tests/unit/plugin_tests.rs (1)
710-736: LGTM!Also applies to: 2287-2317
crates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rs (1)
115-134: LGTM!Also applies to: 732-749, 751-838, 839-871, 872-926, 927-947, 948-1035, 1077-1093
crates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rs (1)
94-111: LGTM!Also applies to: 226-241, 809-855, 932-950
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
|
Closing this combined PR in favor of the four scoped replacement PRs:
Recommended merge order: #633, #634, #635, then #636. Merging all four produces the same Git tree as this combined PR. |
1 participant
Overview
Addresses the applicable Sonar findings reported for
release/0.7(excluding the GitHub Actions findings) and raises merged Rust patch coverage above 96%.Details
release/0.7Sonar inventoryValidation:
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --keep-going -- -D warningsjust test-rustjust --set ci true --set output_dir target/coverage test-rust(3,734 passed)just --set ci true --set output_dir target/coverage test-python(70 Rust unit tests and 614 Python tests passed)just test-gojust test-node(344 passed)uv run pre-commit run --all-filesWhere should the reviewer start?
Start with
Cargo.tomlandclippy.tomlfor the workspace lint policy, then review the production refactors incrates/core/src,crates/adaptive/src, and the corresponding coverage tests.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores