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>
|
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 |
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. |
#### Overview Addresses runtime and tooling findings separated from the broader coverage and complexity work for release/0.7. - [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 - Ensure native asynchronous stream forwarding settles callbacks when chunk serialization or allocation fails. - Keep the native plugin regression coverage with the runtime change so this PR is independently reviewable. - Correct the scope-stack documentation example and small Node adaptive default-value handling. - Document the intentionally safe atomic replacement in the Python plugin build backend. - No breaking API changes. Validation performed on the equivalent combined tree: - uv run pre-commit run --all-files - Native plugin integration tests through just test-rust - Exact four-branch merge tree comparison against the original combined branch #### 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) - Relates to #632 ## Summary by CodeRabbit - **Bug Fixes** - Improved reliability of native asynchronous streaming, including clearer reporting for conversion failures, callback errors, cancellations, panics, and continuation issues. - Strengthened handling of stream completion and error states to prevent silent failures and improve resource cleanup. - **Documentation** - Corrected an API documentation example. - Clarified response-cache configuration documentation and default values. - **Tests** - Expanded coverage for plugin callbacks, streaming, cancellation, errors, and lifecycle behavior. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Eric Evans II (https://github.com/ericevans-nv) URL: #633
#### Overview Adds the non-CLI test coverage extracted from the combined Sonar and coverage work for release/0.7. - [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 - Expand Rust core and adaptive unit and integration coverage. - Cover codec, observability, plugin, logging, worker, subscriber, and runtime edge cases. - Extend FFI, Python-native, Node, and Go binding coverage while preserving existing test patterns. - Refactor high-complexity test functions needed by the cognitive-complexity policy PR. - No breaking API changes. Validation performed: - Branch commit hooks, including formatting, Clippy, Cargo checks, Go formatting and vet, and Node formatting - uv run pre-commit run --all-files on the equivalent combined tree - Rust workspace tests; the CLI global-state-sensitive suite passed with one test thread - Exact four-branch merge tree comparison against the original combined branch #### Where should the reviewer start? Start with crates/plugin/tests/typed_callbacks.rs for the test-complexity refactors, then crates/core/tests/coverage/logging_rotation_tests.rs for representative new coverage. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to #632 ## Summary by CodeRabbit * **Tests** * Expanded coverage for response caching, logging rotation, codec handling, observability, plugin configuration, worker processes, guardrails, and lifecycle management. * Added regression checks for malformed inputs, streaming completion, error handling, status validation, repeated cache updates, and isolated configuration environments. * Improved cross-language coverage across FFI, Python, Node.js, Go, and Rust integrations. * Strengthened validation of API registries, event metadata, telemetry, callbacks, and stream handling. * Refined test organization and shared assertions without changing product behavior. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Eric Evans II (https://github.com/ericevans-nv) URL: #634
#### Overview Raises the CLI component coverage target above 92 percent while excluding prompt-only logic that cannot be exercised reliably in automated coverage runs. - [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 - Separate prompt-driven config editor, plugin editor, and wizard logic from testable state and transformation logic. - Add focused coverage for configuration editing, plugin lifecycle and schemas, installation, server behavior, and process launching. - Update Codecov component paths so untestable interactive prompt modules are excluded from the CLI metric. - Preserve production behavior while making non-interactive logic directly testable. - No breaking API changes. Validation performed: - Branch commit hooks, including formatting, Clippy, Cargo checks, and Codecov YAML validation - uv run pre-commit run --all-files on the equivalent combined tree - All 1,185 CLI library tests passed serially - Exact four-branch merge tree comparison against the original combined branch #### Where should the reviewer start? Start with codecov.yml and crates/cli/src/commands/configure/editor.rs, then compare its prompt module and crates/cli/tests/coverage/commands/configure_editor_tests.rs. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to #632 ## Summary by CodeRabbit * **New Features** * Enhanced the interactive configuration wizard with previews, confirmation, cancellation, defaults, and resume guidance. * Improved configuration editing for gateway limits, providers, logging, file sinks, and secret values. * Expanded plugin configuration with nested fields, lists, maps, JSON, enums, validation, reset, clear, and help actions. * Added schema-aware dynamic plugin editing with secret protection. * **Bug Fixes** * Improved input validation, terminal action handling, interruptions, and configuration errors. * Preserved unrelated settings during configuration updates. * Improved server shutdown error reporting and diagnostics. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Eric Evans II (https://github.com/ericevans-nv) URL: #635
#### Overview Enables Clippy cognitive-complexity enforcement at a threshold of 18 and refactors flagged production functions without suppressing the lint. - [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 - Configure cognitive_complexity as deny in the root Cargo.toml and set the threshold to 18 in clippy.toml. - Split complex adaptive, observability, plugin, stream, Python binding, Go binding, and packaging functions into focused helpers. - Preserve behavior and avoid per-function allow attributes. - This PR is intended to merge last, after the Rust coverage and CLI coverage PRs, because those PRs contain the corresponding test-function refactors required by the newly enforced threshold. - No breaking API changes. Validation performed on the intended combined result: - uv run pre-commit run --all-files, including cargo clippy --workspace --all-targets -- -D warnings - Rust workspace tests; the CLI global-state-sensitive suite passed with one test thread - Exact four-branch merge tree comparison against the original combined branch #### Where should the reviewer start? Start with Cargo.toml and clippy.toml, then review crates/core/src/plugin.rs and crates/python/src/py_callable.rs as representative refactors. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to #632 ## Summary by CodeRabbit - **Refactor** - Improved validation, response caching, streaming, sanitization, plugin configuration, and OpenTelemetry configuration internals without changing expected behavior. - Preserved existing error handling, cache bypass rules, guarded-stream behavior, and telemetry attribute processing. - Improved command-line option parsing for Linux package tooling. - **Chores** - Added workspace linting to flag functions with excessive cognitive complexity. - Clarified licensing headers and lint-suppression reasons. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Maryam Najafian (https://github.com/mnajafian-nv) URL: #636
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