feat: add operational logging events#455
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:XL
|
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:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (5)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (9)**/*.rs📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Files:
**/*.{rs,py}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,js,mjs,cjs,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,go,js,ts,c,h}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,go,js,ts}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*📄 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:
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}⚙️ CodeRabbit configuration file
Files:
{crates/**/src/**/*.rs,python/**/*.py}📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Files:
🛑 Comments failed to post (1)
🔇 Additional comments (7)
WalkthroughThis change adds structured operational logging across CLI lifecycle paths, core observability, plugin activation and workers, backend validation, storage access, and shutdown handling. It also adds logging dependencies, enforces logging contracts, and expands JSONL, lifecycle, retry-state, and deterministic test coverage. ChangesOperational logging
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/core/src/observability/atof.rs (1)
652-683: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not silently acknowledge failed endpoint flushes and closes.
A closed worker channel produces no record at Lines 654/671, while WebSocket drain failure is ignored before sending
done. Consequently, buffered events may remain or be dropped while flush/close appears successful. Emitendpoint_flush_failed/endpoint_close_failedfor these branches and carry the failure into exporter state.As per path instructions, review stream finalization and event lifecycle regressions.
Also applies to: 967-980
🤖 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/src/observability/atof.rs` around lines 652 - 683, Update Endpoint’s flush and close lifecycle in flush and close so failed worker-channel sends and WebSocket drain failures are not treated as successful acknowledgements: emit the corresponding endpoint_flush_failed or endpoint_close_failed event with failure details, and propagate the failure into exporter state. Review stream finalization and event lifecycle handling, including the related close path, so buffered events cannot be silently dropped before completion is reported.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/adaptive/src/redis.rs`:
- Around line 78-97: Change the success logging around get_connection_manager()
so it reports only successful Redis connectivity/authentication, not validated
read_write access. Emit a connection-only event here, or defer
plugin_resource_access_validated until representative GET, SET, and LRANGE
operations succeed; keep the existing connection failure handling intact.
In `@crates/cli/src/commands/mod.rs`:
- Around line 58-62: Update the command-name selection in run_default() so the
no-subcommand path does not always report "serve" when executing doctor or
configure. Derive the effective operation name, or use a neutral "default" name
paired with a separate mode field, while preserving explicit subcommand names
for lifecycle metrics and failure aggregation.
In `@crates/cli/src/error.rs`:
- Around line 66-80: Add a specific match arm in CliError::log_kind for
CliError::Flow(FlowError::GuardrailRejected(_)) returning "guardrail_rejected",
placing it before the generic Self::Flow(_) arm so wrapped guardrail errors
retain their guardrail kind.
In `@crates/cli/src/hooks/response.rs`:
- Around line 113-115: Update the fail-closed error handling around the
hook-forward response to preserve and return the original error variant instead
of wrapping every failure in CliError::Install. Keep the existing contextual
message and logging, while ensuring variants such as Upstream remain
distinguishable to downstream command_failed records and callers.
In `@crates/cli/src/mcp/mod.rs`:
- Around line 50-56: Update the MCP session flow around session::run to capture
its result instead of propagating immediately. Emit mcp_session_closed only when
the run succeeds, emit a redacted mcp_session_failed event on error, then return
the original error unchanged.
In `@crates/cli/src/process/launcher.rs`:
- Around line 147-152: Update the error handling around
execute_live_run_with_dynamic so agent_launch_failed is emitted only for errors
occurring before the agent spawns. Track or distinguish the post-spawn state,
and emit a separate run/exited failure event for gateway startup, supervision,
or cleanup errors returned after spawning while preserving the existing error
details.
In `@crates/cli/src/server/mod.rs`:
- Around line 336-341: Move the server_shutdown_started log statement in the
server startup/shutdown flow to immediately before the axum::serve(...).await
call. Ensure it executes before awaiting the server future, while preserving the
existing target, event, instance_id, and message fields.
- Around line 431-439: Update the shutdown cleanup flow around close_result,
flush_result, and clear_result to record component-specific
server_teardown_failed events for any cleanup errors even when axum::serve
completed successfully. Do not use immediate ? propagation; attempt all
remaining cleanup steps, emit server_stopped only when cleanup succeeds, then
return the appropriate cleanup error.
- Around line 396-426: Pass the server instance identifier into
finish_server_shutdown and propagate it through the terminal shutdown event
logging. Add instance_id to the server_failed, server_teardown_failed, and
server_stopped records, preserving the existing error and teardown details so
concurrent lifecycles remain correlatable.
In `@crates/cli/tests/architecture_tests.rs`:
- Around line 374-405: Extend
operational_direct_stderr_is_limited_to_emergency_and_ui_boundaries to scan the
src trees of the adaptive and pii-redaction crates, applying the same
prohibition on eprintln! used for core. Ensure their backend operational output
is rejected unless covered by the existing intended boundary rules.
- Around line 291-294: Update the contract visitor’s logging-macro detection
around the qualified-path match to also resolve single-segment macros imported
from log, including warn!, info!, error!, debug!, and trace!. Ensure imported
invocations receive the same target, event, privacy-field, and debug/trace
validation, or explicitly reject aliases if resolution is unavailable.
In `@crates/cli/tests/cli_tests.rs`:
- Around line 278-289: Update the lifecycle assertions in the test to resolve
each event’s position in records and assert the expected monotonic ordering:
logging_initialized, command_started, diagnostics_completed, command_completed,
then logging_shutdown_started. Retain the existing missing-event diagnostics
while ensuring later lifecycle events cannot precede earlier ones.
In `@crates/core/src/api/runtime/subscriber_dispatcher.rs`:
- Around line 52-70: The dispatcher failure branches around DISPATCHER
initialization and sender.send must coalesce logs so each terminal failure
transition emits its subscriber_event_dropped or subscriber_dispatcher_failed
record only once. Add shared one-time failure-state tracking at the dispatcher
level, update it when initialization or sending first fails, and suppress
duplicate logs for subsequent events while preserving the existing false/error
behavior.
In `@crates/core/src/observability/atof.rs`:
- Around line 510-524: Remove the exporter_shutdown log emission from
force_flush, and emit it only at the successful end of shutdown after all close
and stored-failure checks pass. Preserve force_flush’s existing result handling
without lifecycle logging, and ensure shutdown emits the event once per
successful close.
In `@crates/core/src/observability/mod.rs`:
- Around line 554-559: Update the invalid-status handling around the
OpenTelemetry status conversion to stop passing the raw other value into the
observability warning. Replace status_code = other with a fixed classification
or bounded, explicitly redacted diagnostic, while preserving the Unset fallback
and existing invalid_status_code event.
In `@crates/core/src/observability/openinference.rs`:
- Around line 430-440: In deregister methods in
crates/core/src/observability/openinference.rs:430-440 and
crates/core/src/observability/otel.rs:424-434, stop labeling a successful
deregister_subscriber(name) result as exporter-specific because it only confirms
that the name existed. Either validate callback ownership before logging or
change the event and message to generic subscriber deregistration wording at
both sites.
In `@crates/core/src/observability/plugin_component.rs`:
- Around line 916-924: Update the lifecycle logging in the pending, validated,
and failed resource-access records to use the same field name, resource_index.
Replace storage_index in the validated/failed paths around the corresponding
resource-access logging while preserving the existing index values and event
behavior.
- Around line 3000-3034: Update the reply handling around reply_rx.recv() so
channel disconnection becomes an Err result rather than an immediate propagated
error. Emit the existing storage_access_failed log and transition access_state
for this case, using a distinct reason such as reply_channel_closed, then return
the resulting error while preserving the current success and upload-failure
handling.
In `@crates/core/src/plugins/nemo_guardrails/python.rs`:
- Around line 604-642: Update the worker shutdown cleanup flow around the child
kill/wait logic to track whether both operations succeed. Emit the
`worker_stopped` event only when both `child.kill()` and `child.wait()` succeed;
when either fails, preserve the existing `worker_cleanup_failed` outcome and
suppress the success log.
In `@crates/core/src/plugins/nemo_guardrails/remote.rs`:
- Around line 234-306: Update execute_remote_check to use the same
resource-access tracking as the chat-completions request path: route its direct
self.client.send() call through the shared tracked-send helper, or invoke
record_access_status on successful responses and record_access_failure on
failures. Ensure tool guardrail requests resolve plugin_resource_access_pending
and emit validated or failed outcomes consistently.
---
Outside diff comments:
In `@crates/core/src/observability/atof.rs`:
- Around line 652-683: Update Endpoint’s flush and close lifecycle in flush and
close so failed worker-channel sends and WebSocket drain failures are not
treated as successful acknowledgements: emit the corresponding
endpoint_flush_failed or endpoint_close_failed event with failure details, and
propagate the failure into exporter state. Review stream finalization and event
lifecycle handling, including the related close path, so buffered events cannot
be silently dropped before completion is reported.
🪄 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: bef90f21-26a3-42c7-b3ba-67ce251329ae
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
crates/adaptive/Cargo.tomlcrates/adaptive/src/redis.rscrates/adaptive/src/runtime/backend.rscrates/cli/Cargo.tomlcrates/cli/src/bootstrap/mod.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/configuration/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/error.rscrates/cli/src/gateway/mod.rscrates/cli/src/hooks/delivery.rscrates/cli/src/hooks/response.rscrates/cli/src/installation/marketplace/mod.rscrates/cli/src/mcp/mod.rscrates/cli/src/process/detached.rscrates/cli/src/process/launcher.rscrates/cli/src/server/mod.rscrates/cli/src/sessions/mod.rscrates/cli/tests/architecture_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/core/src/api/llm.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/core/src/logging/mod.rscrates/core/src/observability/atof.rscrates/core/src/observability/mod.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/worker.rscrates/core/src/plugins/model_pricing.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/shared_runtime.rscrates/core/tests/coverage/logging_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/pii-redaction/Cargo.tomlcrates/pii-redaction/src/component.rscrates/pii-redaction/src/local.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 (24)
**/*.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/adaptive/src/runtime/backend.rscrates/pii-redaction/src/component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/src/error.rscrates/adaptive/src/redis.rscrates/core/src/logging/mod.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/pii-redaction/src/local.rscrates/cli/src/sessions/mod.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/src/hooks/delivery.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/cli/src/process/detached.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/mcp/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/cli/src/commands/root.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/architecture_tests.rscrates/cli/src/server/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/src/hooks/response.rscrates/core/src/api/llm.rscrates/core/tests/coverage/logging_tests.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/installation/marketplace/mod.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.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/adaptive/src/runtime/backend.rscrates/adaptive/Cargo.tomlcrates/core/src/plugins/model_pricing.rscrates/adaptive/src/redis.rscrates/core/src/logging/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/src/api/llm.rscrates/core/tests/coverage/logging_tests.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/adaptive/src/runtime/backend.rscrates/pii-redaction/src/component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/src/error.rscrates/adaptive/src/redis.rscrates/core/src/logging/mod.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/pii-redaction/src/local.rscrates/cli/src/sessions/mod.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/src/hooks/delivery.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/cli/src/process/detached.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/mcp/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/cli/src/commands/root.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/architecture_tests.rscrates/cli/src/server/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/src/hooks/response.rscrates/core/src/api/llm.rscrates/core/tests/coverage/logging_tests.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/installation/marketplace/mod.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.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/adaptive/src/runtime/backend.rscrates/pii-redaction/src/component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/src/error.rscrates/adaptive/src/redis.rscrates/core/src/logging/mod.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/pii-redaction/src/local.rscrates/cli/src/sessions/mod.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/src/hooks/delivery.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/cli/src/process/detached.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/mcp/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/cli/src/commands/root.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/architecture_tests.rscrates/cli/src/server/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/src/hooks/response.rscrates/core/src/api/llm.rscrates/core/tests/coverage/logging_tests.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/installation/marketplace/mod.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.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/adaptive/src/runtime/backend.rscrates/pii-redaction/src/component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/src/error.rscrates/adaptive/src/redis.rscrates/core/src/logging/mod.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/pii-redaction/src/local.rscrates/cli/src/sessions/mod.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/src/hooks/delivery.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/cli/src/process/detached.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/mcp/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/cli/src/commands/root.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/architecture_tests.rscrates/cli/src/server/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/src/hooks/response.rscrates/core/src/api/llm.rscrates/core/tests/coverage/logging_tests.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/installation/marketplace/mod.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.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/adaptive/src/runtime/backend.rscrates/pii-redaction/src/component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/src/error.rscrates/adaptive/src/redis.rscrates/core/src/logging/mod.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/pii-redaction/src/local.rscrates/cli/src/sessions/mod.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/src/hooks/delivery.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/cli/src/process/detached.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/mcp/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/cli/src/commands/root.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/architecture_tests.rscrates/cli/src/server/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/src/hooks/response.rscrates/core/src/api/llm.rscrates/core/tests/coverage/logging_tests.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/installation/marketplace/mod.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/adaptive/src/runtime/backend.rscrates/pii-redaction/src/component.rscrates/core/src/plugins/model_pricing.rscrates/cli/src/error.rscrates/adaptive/src/redis.rscrates/core/src/logging/mod.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/pii-redaction/src/local.rscrates/cli/src/sessions/mod.rscrates/cli/src/hooks/delivery.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/cli/src/process/detached.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/mcp/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/commands/root.rscrates/cli/src/server/mod.rscrates/cli/src/configuration/mod.rscrates/cli/src/gateway/mod.rscrates/cli/src/process/launcher.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/src/hooks/response.rscrates/core/src/api/llm.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/cli/src/installation/marketplace/mod.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.rs
crates/adaptive/**
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Keep
crates/adaptivealigned with the canonical adaptive config schema, built-in section helpers, plugin lifecycle, and validation/report behavior.
Files:
crates/adaptive/src/runtime/backend.rscrates/adaptive/Cargo.tomlcrates/adaptive/src/redis.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/adaptive/src/runtime/backend.rscrates/adaptive/Cargo.tomlcrates/pii-redaction/src/component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/src/error.rscrates/adaptive/src/redis.rscrates/pii-redaction/Cargo.tomlcrates/core/src/logging/mod.rscrates/cli/Cargo.tomlcrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/pii-redaction/src/local.rscrates/cli/src/sessions/mod.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/src/hooks/delivery.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/cli/src/process/detached.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/mcp/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/cli/src/commands/root.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/architecture_tests.rscrates/cli/src/server/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/src/hooks/response.rscrates/core/src/api/llm.rscrates/core/tests/coverage/logging_tests.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/installation/marketplace/mod.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.rs
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/adaptive/src/runtime/backend.rscrates/adaptive/Cargo.tomlcrates/core/src/plugins/model_pricing.rscrates/adaptive/src/redis.rscrates/core/src/logging/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/src/api/llm.rscrates/core/tests/coverage/logging_tests.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.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/adaptive/src/runtime/backend.rscrates/pii-redaction/src/component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/src/error.rscrates/adaptive/src/redis.rscrates/core/src/logging/mod.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/pii-redaction/src/local.rscrates/cli/src/sessions/mod.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/src/hooks/delivery.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/cli/src/process/detached.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/mcp/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/cli/src/commands/root.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/architecture_tests.rscrates/cli/src/server/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/src/hooks/response.rscrates/core/src/api/llm.rscrates/core/tests/coverage/logging_tests.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/cli/src/installation/marketplace/mod.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.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/adaptive/src/runtime/backend.rscrates/core/src/plugins/model_pricing.rscrates/adaptive/src/redis.rscrates/core/src/logging/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/src/api/llm.rscrates/core/tests/coverage/logging_tests.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.rs
**/Cargo.toml
📄 CodeRabbit inference engine (.agents/skills/prepare-code-freeze/SKILL.md)
Confirm or infer the target release version from
upstream/main:Cargo.toml. Derive the release branch asrelease/<major>.<minor>.Keep Rust package names and workspace metadata in
Cargo.tomlinternally consistent across the project.
Files:
crates/adaptive/Cargo.tomlcrates/pii-redaction/Cargo.tomlcrates/cli/Cargo.toml
**/*.toml
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all TOML files using the
#comment form.
Files:
crates/adaptive/Cargo.tomlcrates/pii-redaction/Cargo.tomlcrates/cli/Cargo.toml
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/plugins/model_pricing.rscrates/core/src/logging/mod.rscrates/core/src/observability/mod.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/otel.rscrates/core/src/shared_runtime.rscrates/core/src/observability/openinference.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/src/api/llm.rscrates/core/tests/coverage/logging_tests.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/plugin_component.rscrates/core/tests/unit/observability/atof_tests.rscrates/core/src/plugin.rscrates/core/src/observability/atof.rscrates/core/src/plugin/dynamic/worker.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/cli/tests/coverage/shared/bootstrap_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/tests/cli_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/architecture_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/core/tests/coverage/logging_tests.rscrates/core/tests/unit/observability/atof_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/host.rscrates/core/src/plugin/dynamic/worker.rs
{crates/core/src/plugin/dynamic/**/*.rs,examples/rust-native-plugin/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not pass Rust runtime types, trait objects, futures, or allocator-owned strings across the native dynamic-library boundary.
Files:
crates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/worker.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/host.rscrates/core/src/plugin/dynamic/worker.rs
{crates/core/src/plugin/dynamic/**/*.rs,crates/plugin/**/*.rs,crates/worker/**/*.rs,crates/worker-proto/**/*.rs,python/plugin/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Manifest validation must cover kind, compatibility, load contract, integrity, capability mismatch, and disabled-plugin behavior.
Files:
crates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/worker.rs
crates/core/src/plugin/dynamic/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
The native loader must keep libraries alive until registered callbacks are cleared and must deregister plugin kinds before unload.
Files:
crates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/worker.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/subscriber_dispatcher.rscrates/core/src/api/llm.rs
crates/core/src/observability/{atif,otel,openinference}.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
When changing the core event model, emitted fields, exporter behavior, subscriber config, or lifecycle, keep the ATIF, OpenTelemetry, and OpenInference implementations in sync.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/openinference.rs
crates/core/src/api/{tool,llm}.rs
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Wire the new middleware chain into the execute path in
crates/core/src/api/tool.rsorcrates/core/src/api/llm.rsat the appropriate pipeline stage
Files:
crates/core/src/api/llm.rs
🔇 Additional comments (44)
crates/adaptive/Cargo.toml (1)
18-18: LGTM!crates/core/src/logging/mod.rs (1)
88-94: LGTM!Also applies to: 127-131
crates/cli/src/commands/root.rs (1)
129-146: LGTM!Also applies to: 150-154, 157-163
crates/cli/src/commands/mod.rs (1)
90-131: LGTM!crates/cli/src/mcp/mod.rs (1)
25-29: LGTM!Also applies to: 44-49, 66-71
crates/cli/src/process/launcher.rs (1)
115-145: LGTM!crates/core/tests/unit/dynamic_worker_tests.rs (1)
1443-1443: LGTM!crates/cli/Cargo.toml (1)
49-49: LGTM!crates/pii-redaction/Cargo.toml (1)
22-22: LGTM!crates/cli/src/configuration/mod.rs (1)
95-101: LGTM!Also applies to: 858-865, 896-902
crates/cli/src/diagnostics/mod.rs (1)
67-99: LGTM!Also applies to: 1188-1196, 1215-1221
crates/cli/src/server/mod.rs (1)
91-97: LGTM!Also applies to: 306-313
crates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rs (1)
710-710: LGTM!Also applies to: 721-721, 732-732, 754-754
crates/cli/src/gateway/mod.rs (1)
102-109: LGTM!Also applies to: 782-787
crates/cli/src/hooks/delivery.rs (1)
135-154: LGTM!crates/cli/src/sessions/mod.rs (1)
314-320: LGTM!Also applies to: 510-529, 1343-1349
crates/cli/tests/cli_tests.rs (1)
56-85: LGTM!Also applies to: 297-329
crates/cli/tests/coverage/agents/launcher_tests.rs (1)
1483-1483: LGTM!Also applies to: 1501-1504, 1680-1683, 1752-1755, 1801-1804
crates/cli/tests/coverage/agents/plugin_install_tests.rs (1)
1009-1013: LGTM!Also applies to: 1016-1060
crates/cli/src/bootstrap/mod.rs (1)
94-105: LGTM!Also applies to: 108-135, 275-281, 302-307, 355-380, 439-443
crates/cli/src/installation/marketplace/mod.rs (1)
144-184: LGTM!Also applies to: 186-229, 1090-1099, 1587-1612, 1777-1821
crates/cli/src/process/detached.rs (1)
361-365: LGTM!Also applies to: 388-393
crates/cli/tests/coverage/shared/bootstrap_tests.rs (1)
13-13: LGTM!crates/cli/tests/coverage/shared/doctor_tests.rs (1)
696-696: LGTM!Also applies to: 714-717, 740-740
crates/core/tests/coverage/logging_tests.rs (1)
311-319: LGTM!Also applies to: 530-565, 726-730
crates/core/tests/unit/observability/atof_tests.rs (1)
448-448: LGTM!Also applies to: 1593-1596
crates/core/src/api/llm.rs (2)
363-369: 🎯 Functional CorrectnessVerify the resolved
logfeatures.These calls use
logkey-value fields such asevent = .... Thelogmacro implementation gates key-value support behind thekvfeature, so confirm every affected crate enables it or compilation may fail. (docs.rs)
514-521: 📐 Maintainability & Code QualityRun the required Rust and workspace validation.
Before handoff, run
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust,uv run pre-commit run --all-files, and the repository’s Python, Go, and Node.js test targets.As per coding guidelines, Rust changes require these checks, and
crates/corechanges require the full language matrix.Source: Coding guidelines
crates/core/src/observability/openinference.rs (2)
418-425: 📐 Maintainability & Code QualityEnsure built-in plugin activation emits exporter registration events.
Both built-in paths register raw callbacks through
ctx.register_subscriber(...), bypassing the new subscriberregister()methods.
crates/core/src/observability/openinference.rs#L418-L425: instrument the shared registration path orregister_openinference.crates/core/src/observability/otel.rs#L412-L419: instrument the shared registration path orregister_opentelemetry.
461-470: 📐 Maintainability & Code QualityCover exporter shutdown failures explicitly.
Both methods log only successful shutdowns; verify that the plugin teardown path emits a structured failure event when shutdown returns an error.
crates/core/src/observability/openinference.rs#L461-L470: add or verifyexporter_shutdown_failed.crates/core/src/observability/otel.rs#L455-L464: add or verifyexporter_shutdown_failed.crates/core/src/api/runtime/subscriber_dispatcher.rs (2)
104-116: LGTM!Also applies to: 168-172
104-116: 📐 Maintainability & Code QualityConfirm the required core-runtime validation gates.
Before handoff, verify
cargo fmt --all, strict workspace Clippy,just test-rust,cargo deny check,validate-change, the full language matrix, and pre-commit.As per coding guidelines, core runtime changes must satisfy these validation gates.
Source: Coding guidelines
crates/core/src/observability/atof.rs (1)
390-398: LGTM!Also applies to: 454-477, 643-644, 713-744, 827-835, 859-927, 948-958, 991-1066, 1078-1088, 1107-1108, 1152-1161, 1234-1248, 1259-1267, 1280-1317, 1401-1411
crates/core/src/observability/plugin_component.rs (1)
24-25: LGTM!Also applies to: 943-1002, 1038-1038, 1138-1138, 1260-1286, 2732-2734, 2897-2899, 2977-2979
crates/core/src/shared_runtime.rs (1)
210-216: LGTM!Also applies to: 225-231
crates/adaptive/src/runtime/backend.rs (1)
17-26: LGTM!crates/pii-redaction/src/component.rs (1)
731-737: LGTM!crates/core/src/plugin.rs (1)
911-924: LGTM!Also applies to: 981-993, 1006-1012, 1399-1409, 1439-1444, 1460-1490, 1669-1674, 1687-1693
crates/core/src/plugin/dynamic/host.rs (1)
118-124: LGTM!Also applies to: 212-218, 238-243, 330-334, 418-424
crates/core/src/plugin/dynamic/worker.rs (1)
247-257: LGTM!Also applies to: 268-348, 431-436, 507-520, 610-615, 714-809, 1049-1105, 1124-1230, 1308-1308, 1339-1358
crates/core/src/plugins/model_pricing.rs (1)
77-83: LGTM!crates/core/src/plugins/nemo_guardrails/python.rs (1)
9-9: LGTM!Also applies to: 371-434, 502-508
crates/core/src/plugins/nemo_guardrails/remote.rs (1)
5-5: LGTM!Also applies to: 33-33, 79-112
crates/pii-redaction/src/local.rs (1)
47-92: LGTM!
Signed-off-by: Will Killian <wkillian@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/core/src/plugins/nemo_guardrails/python.rs (1)
505-516: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftReturn the timeout error before cleanup.
request_with_timeout()callsself.shutdown()on the timeout branch, andshutdown()blocks onchild.wait()andwriter.join(). That can exceed the requested timeout and hold a Tokio worker thread; move cleanup to a blocking/background path.🤖 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/src/plugins/nemo_guardrails/python.rs` around lines 505 - 516, Update the timeout branch in request_with_timeout so it returns the timeout FlowError immediately instead of synchronously calling self.shutdown(). Move shutdown cleanup, including child.wait() and writer.join(), onto a blocking or background path that cannot delay the timeout response.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cli/tests/coverage/agents/plugin_install_tests.rs`:
- Around line 233-253: Update
replacement_generation_guard_retains_its_lock_when_marker_is_inaccessible to use
a scoped cleanup guard that restores marker_parent permissions unconditionally,
including during panics or assertion failures. After applying 0o000, verify the
directory is actually inaccessible before proceeding; if running as root makes
that impossible, skip or otherwise avoid claiming coverage of the
inaccessible-marker path. Remove the unconditional restoration unwrap and ensure
cleanup preserves temp-directory accessibility.
In `@crates/core/tests/unit/dynamic_worker_tests.rs`:
- Around line 1091-1174: The test cleanup after subscriber flushing is not
panic-safe, leaving registrations, global context, and runtime ownership
installed when assertions or callbacks panic. In the test setup around
registration_context and the final rollback/reset block, introduce an RAII
cleanup guard that owns the registrations and always calls
rollback_registrations, resets NemoRelayContextState, and invokes
reset_runtime_owner_for_tests during drop, while preserving the runtime mutex
guard until cleanup completes.
In `@crates/core/tests/unit/observability/plugin_component_tests.rs`:
- Around line 80-113: Update start_http_status_server to bound the server
thread’s waits: use a deadline-based or nonblocking accept loop, and configure a
read timeout on the accepted stream before the header/body read_exact calls.
Preserve the existing request parsing and response behavior while ensuring
failures to connect or send a complete request cause the thread to exit and let
the test join return.
---
Outside diff comments:
In `@crates/core/src/plugins/nemo_guardrails/python.rs`:
- Around line 505-516: Update the timeout branch in request_with_timeout so it
returns the timeout FlowError immediately instead of synchronously calling
self.shutdown(). Move shutdown cleanup, including child.wait() and
writer.join(), onto a blocking or background path that cannot delay the timeout
response.
🪄 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: b6e2596d-5b62-41e9-8ab7-3e1802e3d17d
📒 Files selected for processing (33)
crates/adaptive/tests/integration/redis_tests.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/cli/src/error.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/error_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/tests/coverage/shared/test_support.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/tests/coverage/logging_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/llm_api_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/plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/tests/unit/shared_tests.rscrates/pii-redaction/tests/unit/component_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/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/error_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/error.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/shared/session_tests.rscrates/adaptive/tests/integration/redis_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/pii-redaction/tests/unit/component_tests.rscrates/core/tests/coverage/logging_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_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/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/src/plugin/dynamic/host.rscrates/adaptive/tests/integration/redis_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/tests/coverage/logging_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_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/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/tests/coverage/logging_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_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/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/error_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/error.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/shared/session_tests.rscrates/adaptive/tests/integration/redis_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/pii-redaction/tests/unit/component_tests.rscrates/core/tests/coverage/logging_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_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/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/error_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/error.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/shared/session_tests.rscrates/adaptive/tests/integration/redis_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/pii-redaction/tests/unit/component_tests.rscrates/core/tests/coverage/logging_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_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/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/error_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/error.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/shared/session_tests.rscrates/adaptive/tests/integration/redis_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/pii-redaction/tests/unit/component_tests.rscrates/core/tests/coverage/logging_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_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/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/error_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/error.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/shared/session_tests.rscrates/adaptive/tests/integration/redis_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/pii-redaction/tests/unit/component_tests.rscrates/core/tests/coverage/logging_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_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/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/error_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/error.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/shared/session_tests.rscrates/adaptive/tests/integration/redis_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/pii-redaction/tests/unit/component_tests.rscrates/core/tests/coverage/logging_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_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/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/src/plugin/dynamic/host.rscrates/adaptive/tests/integration/redis_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/tests/coverage/logging_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_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.
Files:
crates/core/tests/unit/plugin_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/error_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/error.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/shared/session_tests.rscrates/adaptive/tests/integration/redis_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/pii-redaction/tests/unit/component_tests.rscrates/core/tests/coverage/logging_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_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/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/core/src/plugin/dynamic/host.rscrates/adaptive/tests/integration/redis_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/tests/coverage/logging_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_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/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/error_tests.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/tests/unit/codec/response_tests.rscrates/core/tests/integration/subscriber_dispatcher_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/shared_tests.rscrates/cli/tests/coverage/shared/test_support.rscrates/adaptive/tests/integration/runtime_integration_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/component_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/core/tests/unit/llm_api_tests.rscrates/core/tests/coverage/logging_sink_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/adaptive/tests/integration/redis_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rscrates/pii-redaction/tests/unit/component_tests.rscrates/core/tests/coverage/logging_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/observability/atof_tests.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/cli/src/error.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugins/nemo_guardrails/python.rs
crates/adaptive/**
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Keep
crates/adaptivealigned with the canonical adaptive config schema, built-in section helpers, plugin lifecycle, and validation/report behavior.
Files:
crates/adaptive/tests/integration/runtime_integration_tests.rscrates/adaptive/tests/integration/redis_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/host.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/host.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/host.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/host.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/host.rs
🔇 Additional comments (36)
crates/cli/src/error.rs (2)
66-80: Preserve nested guardrail error classification.Line 79 still maps
CliError::Flow(FlowError::GuardrailRejected(_))to"runtime", despite response handling recognizing it as a guardrail rejection. Add the specific nested arm beforeSelf::Flow(_).
156-158: LGTM!crates/adaptive/tests/integration/redis_tests.rs (1)
11-11: LGTM!Also applies to: 32-32, 45-57, 79-79
crates/adaptive/tests/integration/runtime_integration_tests.rs (1)
7-7: LGTM!Also applies to: 51-60, 67-67
crates/cli/tests/coverage/agents/launcher_tests.rs (1)
21-21: LGTM!Also applies to: 1754-1757, 1792-1808
crates/core/tests/integration/worker_plugin_tests.rs (1)
40-43: LGTM!Also applies to: 1243-1243
crates/core/tests/unit/plugins/nemo_guardrails/remote_coverage_tests.rs (1)
17-18: LGTM!Also applies to: 712-756, 1043-1054
crates/pii-redaction/tests/unit/component_tests.rs (1)
27-29: LGTM!Also applies to: 40-52, 74-74, 479-508
crates/cli/tests/coverage/agents/plugin_install_tests.rs (2)
218-218: LGTM!Also applies to: 931-931, 2021-2021
255-266: LGTM!Also applies to: 1045-1094, 2062-2095
crates/cli/tests/coverage/commands/main_tests.rs (1)
16-26: LGTM!crates/cli/tests/coverage/shared/bootstrap_tests.rs (1)
38-38: LGTM!Also applies to: 47-47, 90-90
crates/core/tests/unit/codec/response_tests.rs (1)
776-777: LGTM!crates/core/tests/unit/llm_api_tests.rs (3)
35-37: LGTM!
685-707: LGTM!
655-684: 🩺 Stability & AvailabilityNo cleanup change needed.
reset_global()already callsreset_runtime_owner_for_tests(), which removesNEMO_RELAY_RUNTIME_OWNERand resets the runtime-owner controller, so this test does not leak process-global state.> Likely an incorrect or invalid review comment.crates/core/tests/unit/observability/atof_tests.rs (3)
29-33: LGTM!Also applies to: 571-571, 612-612, 645-645, 1470-1470, 1566-1566, 1671-1674, 1699-1700, 1765-1767
1499-1518: LGTM!Also applies to: 1603-1669, 1717-1763
447-457: 📐 Maintainability & Code QualityNo truncation-helper regression here
This test only exercisesAtofEndpointFieldNamePolicy::parse("unknown"); there is notruncate_log_bodyhelper or ellipsis-boundary logic in the current observability code, so the truncation coverage concern doesn’t apply.> Likely an incorrect or invalid review comment.crates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rs (2)
122-123: LGTM!Also applies to: 190-207
209-247: 🩺 Stability & AvailabilityNo process leak here
request_with_timeout()already callsshutdown(), andshutdown()kills and waits on the child before joining the writer thread, so this test does not leak asleep 60orphan or zombie.> Likely an incorrect or invalid review comment.crates/cli/tests/coverage/shared/error_tests.rs (1)
1-72: LGTM!crates/cli/tests/coverage/shared/session_tests.rs (1)
6217-6217: LGTM!Also applies to: 6861-6861
crates/core/src/plugin/dynamic/host.rs (1)
118-124: LGTM!Also applies to: 212-218, 238-243, 330-334, 418-424, 447-448
crates/core/tests/unit/dynamic_worker_tests.rs (1)
7-7: LGTM!Also applies to: 31-38, 75-75, 89-89, 193-193, 235-235, 331-331, 355-355, 375-375, 542-542, 570-570, 631-631, 689-689, 865-865, 932-932, 990-990, 1049-1090, 1177-1179, 1198-1198, 1342-1342, 1418-1424, 1597-1597, 1609-1609
crates/core/tests/unit/observability/plugin_component_tests.rs (1)
127-129: LGTM!Also applies to: 1104-1127
crates/core/tests/unit/plugins/nemo_guardrails/component_tests.rs (1)
44-45: LGTM!crates/cli/tests/coverage/shared/gateway_tests.rs (1)
18-18: LGTM!crates/cli/tests/coverage/shared/mcp_tests.rs (1)
24-26: LGTM!Also applies to: 55-57
crates/core/tests/integration/subscriber_dispatcher_tests.rs (1)
18-19: LGTM!crates/core/tests/unit/observability/openinference_tests.rs (1)
46-47: LGTM!Also applies to: 3162-3170
crates/core/tests/unit/observability/otel_tests.rs (1)
287-288: LGTM!crates/core/tests/unit/plugin_tests.rs (1)
425-426: LGTM!crates/core/src/plugins/nemo_guardrails/python.rs (1)
619-646: Suppressworker_stoppedwhen cleanup fails.This still emits a success lifecycle event after
kill()orwait()fails, matching the unresolved prior-review finding.crates/cli/tests/coverage/shared/server_tests.rs (1)
201-213: 📐 Maintainability & Code QualityProvide the required Rust and core-runtime validation results.
crates/cli/tests/coverage/shared/server_tests.rs#L201-L213: include this test change in Rust formatting, lint, test, and pre-commit results.crates/cli/tests/coverage/shared/test_support.rs#L14-L21: include the global logging setup in the same validation run.crates/core/src/plugins/nemo_guardrails/python.rs#L492-L516: additionally runvalidate-change,cargo deny check, and the Rust/Python/Go/Node matrix required for core changes.crates/core/tests/coverage/logging_sink_tests.rs#L21-L35: include logging-sink coverage in the Rust test result.crates/core/tests/coverage/logging_tests.rs#L204-L316: include logging configuration coverage in the Rust test result.crates/core/tests/integration/native_plugin_tests.rs#L1770-L1772: include native-plugin integration coverage in the Rust test result.crates/core/tests/unit/shared_tests.rs#L75-L76: include runtime-owner coverage in the Rust test result.Please attach successful output for
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust, anduv run pre-commit run --all-files.As per coding guidelines, “If any Rust code changed, always run
just test-rust,” “runcargo fmt --all,” and “runcargo clippy --workspace --all-targets -- -D warnings”; core changes also requirevalidate-change,cargo deny check, and the full language matrix.Source: Coding guidelines
crates/core/tests/unit/shared_tests.rs (1)
156-164: 🎯 Functional CorrectnessIncorrect concern:
pid + 1is fine here.ensure_process_runtime_owner()treats any owner with a different PID as stale and reclaims it, so this test is not relying on PID liveness.> Likely an incorrect or invalid review comment.
Signed-off-by: Will Killian <wkillian@nvidia.com>
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 (8)
crates/core/src/plugin/dynamic/host.rs (1)
118-124: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftLog terminal activation failures on every error path.
After this start event, several
?paths can return without a matching failure record;plugin_rollback_failedonly covers incomplete rollback. Emit a privacy-safedynamic_plugin_activation_failedevent for lease, load, initialization, and clean-rollback failures.As per path instructions, review core runtime event lifecycle regressions.
🤖 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/src/plugin/dynamic/host.rs` around lines 118 - 124, Update the dynamic plugin activation flow after the dynamic_plugin_activation_started event to emit a privacy-safe dynamic_plugin_activation_failed event on every terminal error path, including lease, load, initialization, and clean-rollback failures. Ensure all relevant ? returns and explicit failures in the activation function are covered, while preserving plugin_rollback_failed for incomplete rollback.Source: Path instructions
crates/cli/tests/architecture_tests.rs (1)
291-328: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winResolve or reject aliased
logmacro imports.
use log::warn as audit_warn; audit_warn!(...)bypasses every contract check because only the final macro segment is matched. ResolveItemUsealiases or explicitly reject aliases in production sources.🤖 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/cli/tests/architecture_tests.rs` around lines 291 - 328, Update the production macro inspection logic around the final-segment macro matching to account for aliased imports: resolve `ItemUse` bindings for `log` macros before applying target, event, privacy-field, and debug/trace checks, or explicitly record aliased `log` macros as failures. Ensure calls such as `audit_warn!` cannot bypass the existing contract checks.crates/cli/src/error.rs (1)
160-163: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winKeep Rust tests out of
src.Move this test harness to the crate’s
tests/tree while preserving the required access to private APIs.As per coding guidelines, Rust tests belong in crate
tests/trees and must not be added undersrc.🤖 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/cli/src/error.rs` around lines 160 - 163, Move the #[cfg(test)] tests module declaration from error.rs into the crate’s tests/ tree, preserving its inclusion of shared/error_tests.rs and the required access to private APIs. Remove the test harness from src while keeping the existing test behavior and coverage path intact.Source: Coding guidelines
crates/cli/src/gateway/mod.rs (1)
789-811: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPreserve explicit dispatch targets on rewrite fallback.
When body serialization fails, this early return occurs before
override_url,override_route, andcredential_policyare computed. The request therefore falls back to the original route while retaining source credentials, even when an explicit target was selected. This can route data to the wrong backend and cross credential boundaries.Parse target metadata before serialization, or preserve the computed target and credential policy in the fallback; add regression coverage for URL and route overrides.
🤖 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/cli/src/gateway/mod.rs` around lines 789 - 811, Update the rewrite flow around the serialization fallback in the request-building function so explicit override_url, override_route, and credential_policy metadata is resolved before serde_json::to_vec can return early, or otherwise reuse the computed target values in that fallback. Ensure serialization failure preserves the selected backend URL, route, and credential policy instead of reverting to the source target, and add regression coverage for URL and route overrides.crates/cli/src/mcp/mod.rs (1)
25-29: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEmit a terminal MCP outcome for setup failures.
mcp_session_startedis logged before gateway resolution, acquisition, andspawn_stdin_reader. Any of those failures exits via?before eithermcp_session_closedormcp_session_failed, leaving lifecycle records unmatched. Wrap setup and session execution in one outcome path, or log the failure at each setup boundary.🤖 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/cli/src/mcp/mod.rs` around lines 25 - 29, Move the mcp_session_started lifecycle handling in the MCP session flow so gateway resolution, acquisition, spawn_stdin_reader, and session execution share a terminal outcome path. Ensure every setup or execution error emits mcp_session_failed, while successful completion emits mcp_session_closed, with no unmatched mcp_session_started records.crates/cli/tests/coverage/shared/server_tests.rs (1)
202-213: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the original listener error, not only its wrapper variant.
CliError::Io(_)would also accept an unrelated teardown I/O error, so this does not prove"listener failed"was preserved.Proposed fix
- assert!(matches!(error, CliError::Io(_))); + let CliError::Io(error) = error else { + panic!("expected listener I/O error"); + }; + assert_eq!(error.to_string(), "listener failed");As per path instructions, “Tests should cover the behavior promised by the changed API surface.”
🤖 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/cli/tests/coverage/shared/server_tests.rs` around lines 202 - 213, Strengthen failed_server_result_is_reported_after_successful_teardown by asserting that the returned CliError::Io contains the original “listener failed” message, rather than matching only the wrapper variant. Keep the existing teardown flow and verify the preserved underlying error specifically.Source: Path instructions
crates/core/src/plugins/nemo_guardrails/remote.rs (1)
259-284: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve resource-access validation only from the pending state.
Unconditional
swaplets alternating or concurrent requests emit repeatedplugin_resource_access_failedandplugin_resource_access_validatedoutcomes. Usecompare_exchange(0, outcome, …)so the first observed result resolves the pending event exactly once.Proposed fix
- if self.access_state.swap(2, Ordering::AcqRel) != 2 { + if self + .access_state + .compare_exchange(0, 2, Ordering::AcqRel, Ordering::Acquire) + .is_ok() + { ... - if self.access_state.swap(1, Ordering::AcqRel) == 1 { + if self + .access_state + .compare_exchange(0, 1, Ordering::AcqRel, Ordering::Acquire) + .is_err() + { return; }As per path instructions, review core Rust changes for “event lifecycle regressions.”
🤖 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/src/plugins/nemo_guardrails/remote.rs` around lines 259 - 284, Update record_access_status and record_access_failure to resolve access_state only when it is still pending (0): replace unconditional swaps with compare_exchange(0, outcome, Ordering::AcqRel, appropriate failure ordering). Emit each validation or failure event only when that compare-exchange succeeds, preserving the first observed result and preventing later or concurrent requests from emitting additional outcomes.Source: Path instructions
crates/core/src/plugins/nemo_guardrails/python.rs (1)
618-644: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not report a successful stop when the child mutex is poisoned.
If
self.child.lock()fails, cleanup is skipped whilecleanup_succeededremains true. This can logworker_stoppedwith a live child, and idempotency prevents retrying cleanup. Recover the poisoned guard withinto_inner()or mark cleanup failed.As per path instructions, review core Rust changes for “event lifecycle regressions.”
🤖 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/src/plugins/nemo_guardrails/python.rs` around lines 618 - 644, Update the cleanup logic around self.child.lock() so a poisoned mutex does not leave cleanup_succeeded true. Either recover the guard with into_inner() and perform kill/wait cleanup, or explicitly set cleanup_succeeded to false when locking fails, ensuring worker_stopped is not reported and cleanup can be retried.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/cli/src/error.rs`:
- Around line 160-163: Move the #[cfg(test)] tests module declaration from
error.rs into the crate’s tests/ tree, preserving its inclusion of
shared/error_tests.rs and the required access to private APIs. Remove the test
harness from src while keeping the existing test behavior and coverage path
intact.
In `@crates/cli/src/gateway/mod.rs`:
- Around line 789-811: Update the rewrite flow around the serialization fallback
in the request-building function so explicit override_url, override_route, and
credential_policy metadata is resolved before serde_json::to_vec can return
early, or otherwise reuse the computed target values in that fallback. Ensure
serialization failure preserves the selected backend URL, route, and credential
policy instead of reverting to the source target, and add regression coverage
for URL and route overrides.
In `@crates/cli/src/mcp/mod.rs`:
- Around line 25-29: Move the mcp_session_started lifecycle handling in the MCP
session flow so gateway resolution, acquisition, spawn_stdin_reader, and session
execution share a terminal outcome path. Ensure every setup or execution error
emits mcp_session_failed, while successful completion emits mcp_session_closed,
with no unmatched mcp_session_started records.
In `@crates/cli/tests/architecture_tests.rs`:
- Around line 291-328: Update the production macro inspection logic around the
final-segment macro matching to account for aliased imports: resolve `ItemUse`
bindings for `log` macros before applying target, event, privacy-field, and
debug/trace checks, or explicitly record aliased `log` macros as failures.
Ensure calls such as `audit_warn!` cannot bypass the existing contract checks.
In `@crates/cli/tests/coverage/shared/server_tests.rs`:
- Around line 202-213: Strengthen
failed_server_result_is_reported_after_successful_teardown by asserting that the
returned CliError::Io contains the original “listener failed” message, rather
than matching only the wrapper variant. Keep the existing teardown flow and
verify the preserved underlying error specifically.
In `@crates/core/src/plugin/dynamic/host.rs`:
- Around line 118-124: Update the dynamic plugin activation flow after the
dynamic_plugin_activation_started event to emit a privacy-safe
dynamic_plugin_activation_failed event on every terminal error path, including
lease, load, initialization, and clean-rollback failures. Ensure all relevant ?
returns and explicit failures in the activation function are covered, while
preserving plugin_rollback_failed for incomplete rollback.
In `@crates/core/src/plugins/nemo_guardrails/python.rs`:
- Around line 618-644: Update the cleanup logic around self.child.lock() so a
poisoned mutex does not leave cleanup_succeeded true. Either recover the guard
with into_inner() and perform kill/wait cleanup, or explicitly set
cleanup_succeeded to false when locking fails, ensuring worker_stopped is not
reported and cleanup can be retried.
In `@crates/core/src/plugins/nemo_guardrails/remote.rs`:
- Around line 259-284: Update record_access_status and record_access_failure to
resolve access_state only when it is still pending (0): replace unconditional
swaps with compare_exchange(0, outcome, Ordering::AcqRel, appropriate failure
ordering). Emit each validation or failure event only when that compare-exchange
succeeds, preserving the first observed result and preventing later or
concurrent requests from emitting additional outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3ddcad26-8f49-4f93-8ef8-44d3539a1c67
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
crates/adaptive/src/drain.rscrates/adaptive/src/redis.rscrates/adaptive/src/runtime/features.rscrates/cli/Cargo.tomlcrates/cli/src/commands/mod.rscrates/cli/src/error.rscrates/cli/src/gateway/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/mcp/mod.rscrates/cli/src/process/launcher.rscrates/cli/src/server/mod.rscrates/cli/tests/architecture_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/core/src/observability/atof.rscrates/core/src/observability/mod.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/tests/unit/dynamic_worker_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/local_python_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 (23)
**/*.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/observability/otel.rscrates/core/tests/unit/observability/otel_tests.rscrates/adaptive/src/runtime/features.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/adaptive/src/drain.rscrates/core/src/observability/openinference.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/adaptive/src/redis.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/cli/src/error.rscrates/cli/src/hooks/response.rscrates/cli/tests/cli_tests.rscrates/cli/src/mcp/mod.rscrates/core/src/plugin/dynamic/host.rscrates/cli/src/gateway/mod.rscrates/cli/tests/architecture_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.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/observability/otel.rscrates/core/tests/unit/observability/otel_tests.rscrates/adaptive/src/runtime/features.rscrates/adaptive/src/drain.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/adaptive/src/redis.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.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/observability/otel.rscrates/core/tests/unit/observability/otel_tests.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/src/observability/otel.rscrates/core/tests/unit/observability/otel_tests.rscrates/adaptive/src/runtime/features.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/adaptive/src/drain.rscrates/core/src/observability/openinference.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/adaptive/src/redis.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/cli/src/error.rscrates/cli/src/hooks/response.rscrates/cli/tests/cli_tests.rscrates/cli/src/mcp/mod.rscrates/core/src/plugin/dynamic/host.rscrates/cli/src/gateway/mod.rscrates/cli/tests/architecture_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.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/src/observability/otel.rscrates/core/tests/unit/observability/otel_tests.rscrates/adaptive/src/runtime/features.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/adaptive/src/drain.rscrates/core/src/observability/openinference.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/adaptive/src/redis.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/cli/src/error.rscrates/cli/src/hooks/response.rscrates/cli/tests/cli_tests.rscrates/cli/src/mcp/mod.rscrates/core/src/plugin/dynamic/host.rscrates/cli/src/gateway/mod.rscrates/cli/tests/architecture_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.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/src/observability/otel.rscrates/core/tests/unit/observability/otel_tests.rscrates/adaptive/src/runtime/features.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/adaptive/src/drain.rscrates/core/src/observability/openinference.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/adaptive/src/redis.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/cli/src/error.rscrates/cli/src/hooks/response.rscrates/cli/tests/cli_tests.rscrates/cli/src/mcp/mod.rscrates/core/src/plugin/dynamic/host.rscrates/cli/src/gateway/mod.rscrates/cli/tests/architecture_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.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/src/observability/otel.rscrates/core/tests/unit/observability/otel_tests.rscrates/adaptive/src/runtime/features.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/adaptive/src/drain.rscrates/core/src/observability/openinference.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/adaptive/src/redis.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/cli/src/error.rscrates/cli/src/hooks/response.rscrates/cli/tests/cli_tests.rscrates/cli/src/mcp/mod.rscrates/core/src/plugin/dynamic/host.rscrates/cli/src/gateway/mod.rscrates/cli/tests/architecture_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.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/observability/otel.rscrates/adaptive/src/runtime/features.rscrates/adaptive/src/drain.rscrates/core/src/observability/openinference.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/adaptive/src/redis.rscrates/cli/src/error.rscrates/cli/src/hooks/response.rscrates/cli/src/mcp/mod.rscrates/core/src/plugin/dynamic/host.rscrates/cli/src/gateway/mod.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/cli/src/server/mod.rscrates/cli/src/process/launcher.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.rs
crates/core/src/observability/{atif,otel,openinference}.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
When changing the core event model, emitted fields, exporter behavior, subscriber config, or lifecycle, keep the ATIF, OpenTelemetry, and OpenInference implementations in sync.
Files:
crates/core/src/observability/otel.rscrates/core/src/observability/openinference.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/src/observability/otel.rscrates/cli/Cargo.tomlcrates/core/tests/unit/observability/otel_tests.rscrates/adaptive/src/runtime/features.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/adaptive/src/drain.rscrates/core/src/observability/openinference.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/adaptive/src/redis.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/cli/src/error.rscrates/cli/src/hooks/response.rscrates/cli/tests/cli_tests.rscrates/cli/src/mcp/mod.rscrates/core/src/plugin/dynamic/host.rscrates/cli/src/gateway/mod.rscrates/cli/tests/architecture_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.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/observability/otel.rscrates/core/tests/unit/observability/otel_tests.rscrates/adaptive/src/runtime/features.rscrates/adaptive/src/drain.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/adaptive/src/redis.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.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/core/src/observability/otel.rscrates/core/tests/unit/observability/otel_tests.rscrates/adaptive/src/runtime/features.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/adaptive/src/drain.rscrates/core/src/observability/openinference.rscrates/cli/src/commands/mod.rscrates/core/src/observability/mod.rscrates/adaptive/src/redis.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/cli/src/error.rscrates/cli/src/hooks/response.rscrates/cli/tests/cli_tests.rscrates/cli/src/mcp/mod.rscrates/core/src/plugin/dynamic/host.rscrates/cli/src/gateway/mod.rscrates/cli/tests/architecture_tests.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/process/launcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.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/observability/otel.rscrates/core/tests/unit/observability/otel_tests.rscrates/adaptive/src/runtime/features.rscrates/adaptive/src/drain.rscrates/core/src/observability/openinference.rscrates/core/src/observability/mod.rscrates/adaptive/src/redis.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugins/nemo_guardrails/python.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/core/src/api/runtime/subscriber_dispatcher.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/nemo_guardrails/remote.rscrates/core/src/observability/atof.rs
**/Cargo.toml
📄 CodeRabbit inference engine (.agents/skills/prepare-code-freeze/SKILL.md)
Confirm or infer the target release version from
upstream/main:Cargo.toml. Derive the release branch asrelease/<major>.<minor>.Keep Rust package names and workspace metadata in
Cargo.tomlinternally consistent across the project.
Files:
crates/cli/Cargo.toml
**/*.toml
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all TOML files using the
#comment form.
Files:
crates/cli/Cargo.toml
{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/observability/otel_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/architecture_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/core/tests/unit/observability/openinference_tests.rscrates/core/tests/unit/observability/plugin_component_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rs
crates/adaptive/**
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Keep
crates/adaptivealigned with the canonical adaptive config schema, built-in section helpers, plugin lifecycle, and validation/report behavior.
Files:
crates/adaptive/src/runtime/features.rscrates/adaptive/src/drain.rscrates/adaptive/src/redis.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/host.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/host.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/host.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/host.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/host.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/subscriber_dispatcher.rs
🔇 Additional comments (34)
crates/adaptive/src/redis.rs (1)
58-97: LGTM!crates/adaptive/src/runtime/features.rs (1)
395-402: LGTM!Also applies to: 438-442
crates/cli/src/server/mod.rs (1)
94-116: LGTM!Also applies to: 228-350, 393-483, 523-599
crates/cli/tests/architecture_tests.rs (1)
404-424: LGTM!crates/cli/tests/cli_tests.rs (1)
290-306: LGTM!crates/cli/tests/coverage/agents/launcher_tests.rs (1)
20-41: LGTM!Also applies to: 252-310, 744-867, 906-992, 1808-1880
crates/core/src/observability/atof.rs (1)
390-398: LGTM!Also applies to: 454-477, 510-515, 550-564, 1001-1043
crates/core/src/plugin/dynamic/host.rs (1)
212-243: LGTM!Also applies to: 329-334, 417-424, 430-431
crates/adaptive/src/drain.rs (1)
182-187: LGTM!Also applies to: 200-209, 225-229
crates/cli/src/commands/mod.rs (1)
58-62: LGTM!Also applies to: 90-131
crates/cli/src/error.rs (1)
37-38: LGTM!Also applies to: 73-73, 82-82, 123-123
crates/cli/src/gateway/mod.rs (1)
20-22: LGTM!Also applies to: 68-74, 103-110, 140-140, 262-262, 444-444, 695-735, 744-750, 784-786, 830-835, 851-853, 862-864, 977-978, 1121-1122
crates/cli/src/hooks/response.rs (1)
22-29: LGTM!Also applies to: 47-52, 61-66, 79-128
crates/cli/src/mcp/mod.rs (1)
51-69: LGTM!Also applies to: 85-103
crates/cli/src/process/launcher.rs (1)
115-155: LGTM!Also applies to: 179-186, 357-367, 437-464, 578-585
crates/cli/Cargo.toml (1)
70-70: LGTM!crates/core/src/api/runtime/subscriber_dispatcher.rs (2)
54-60: Coalesce disconnected-send warnings too.
sender.send(message)can keep failing after the dispatcher exits, and every later event still emitssubscriber_event_dropped. Gate this terminal transition with the same one-time failure state (or a separate atomic state) to avoid the remaining log storm.
14-37: LGTM!Also applies to: 66-75, 107-119, 171-175
crates/core/src/observability/plugin_component.rs (2)
3000-3040: Finalize request-channel failures as resource-access failures.The new reply-channel branch is covered, but
self.sender.send(...)still returns beforeaccess_stateandstorage_access_failedare updated. Route it through this result path withreason = "request_channel_closed".
1038-1038: LGTM!Also applies to: 1260-1288, 2734-2734, 2897-2899, 2977-2979
crates/cli/tests/coverage/agents/plugin_install_tests.rs (1)
238-260: LGTM!Also applies to: 1059-1105
crates/core/src/observability/mod.rs (1)
75-85: LGTM!Also applies to: 565-573, 582-593
crates/core/src/observability/openinference.rs (1)
28-29: LGTM!Also applies to: 419-470, 664-665, 735-736, 771-772
crates/core/src/observability/otel.rs (1)
28-29: LGTM!Also applies to: 413-464, 657-658, 729-730, 757-758
crates/core/tests/unit/observability/openinference_tests.rs (1)
2366-2457: LGTM!crates/core/tests/unit/observability/otel_tests.rs (1)
287-288: LGTM!Also applies to: 1054-1145
crates/cli/tests/coverage/shared/gateway_tests.rs (1)
12-12: LGTM!Also applies to: 23-29, 103-121, 123-264, 458-476, 577-577, 595-715, 743-743, 778-778, 891-894, 1085-1088, 1119-1131, 1209-1325, 1552-1552, 1589-1589
crates/cli/tests/coverage/shared/mcp_tests.rs (1)
25-25: LGTM!Also applies to: 56-56, 426-426
crates/cli/tests/coverage/shared/server_tests.rs (1)
413-413: LGTM!Also applies to: 438-509, 521-521, 589-589, 2424-2507, 3025-3027
crates/core/src/plugins/nemo_guardrails/python.rs (1)
371-371: LGTM!Also applies to: 374-435, 492-617
crates/core/src/plugins/nemo_guardrails/remote.rs (1)
33-33: LGTM!Also applies to: 79-112, 227-257, 565-594
crates/core/tests/unit/dynamic_worker_tests.rs (1)
1050-1190: LGTM!crates/core/tests/unit/observability/plugin_component_tests.rs (1)
81-134: LGTM!crates/core/tests/unit/plugins/nemo_guardrails/local_python_tests.rs (1)
4-37: LGTM!Also applies to: 141-142, 209-267, 573-763
Signed-off-by: Will Killian <wkillian@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cli/src/hooks/response.rs (1)
61-66: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the verified transport error kind.
This path converts a transport failure to
CliError::Installbeforehandle_hook_failure()can wrap it. DownstreamHookDelivery::log_kind()will therefore reporterror_kind="install"instead of the original transport/upstream category. Preserve the typed error and add display context only through the wrapper.🤖 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/cli/src/hooks/response.rs` around lines 61 - 66, Update the Err(error) branch around handle_hook_failure so the original typed verified-transport error is preserved while adding the “verified hook forward failed” display context through the existing error-wrapping mechanism, rather than converting it to CliError::Install. Ensure handle_hook_failure and downstream HookDelivery::log_kind() still receive the original transport/upstream error kind.
🤖 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/cli/tests/coverage/shared/error_tests.rs`:
- Around line 53-58: Extend the error cases around CliError::HookDelivery with a
nested FlowError::GuardrailRejected source, and assert that
guardrail_rejection_reason() is forwarded through the wrapper. Keep the existing
Install case and verify the wrapped guardrail path is classified and exposes its
rejection reason rather than falling back to the generic runtime kind.
---
Outside diff comments:
In `@crates/cli/src/hooks/response.rs`:
- Around line 61-66: Update the Err(error) branch around handle_hook_failure so
the original typed verified-transport error is preserved while adding the
“verified hook forward failed” display context through the existing
error-wrapping mechanism, rather than converting it to CliError::Install. Ensure
handle_hook_failure and downstream HookDelivery::log_kind() still receive the
original transport/upstream error kind.
🪄 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: 9aebe15c-b2a8-4e40-a412-701b9b2b2429
📒 Files selected for processing (4)
crates/cli/src/error.rscrates/cli/src/hooks/response.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/error_tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/*.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/cli/tests/coverage/shared/error_tests.rscrates/cli/src/server/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/error.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/tests/coverage/shared/error_tests.rscrates/cli/src/server/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/error.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/cli/tests/coverage/shared/error_tests.rscrates/cli/src/server/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/error.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/cli/tests/coverage/shared/error_tests.rscrates/cli/src/server/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/error.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/cli/tests/coverage/shared/error_tests.rscrates/cli/src/server/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/error.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/cli/tests/coverage/shared/error_tests.rscrates/cli/src/server/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/error.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/cli/tests/coverage/shared/error_tests.rscrates/cli/src/server/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/error.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/cli/tests/coverage/shared/error_tests.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/cli/src/server/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/error.rs
🔇 Additional comments (4)
crates/cli/src/error.rs (1)
53-57: LGTM!Also applies to: 73-101, 167-170
crates/cli/src/hooks/response.rs (1)
22-29: LGTM!Also applies to: 47-52, 79-130
crates/cli/src/server/mod.rs (1)
94-100: LGTM!Also applies to: 314-321, 336-341, 350-350, 397-483, 576-586
crates/cli/tests/coverage/shared/error_tests.rs (1)
12-82: 📐 Maintainability & Code QualityRun the Rust validation before handoff.
just test-rust,cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, anduv run pre-commit run --all-files.
Signed-off-by: Will Killian <wkillian@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cli/src/gateway/mod.rs (1)
936-947: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward configured auth even when env auth is disabled
inject_provider_auth_with_envreturns beforeconfigured_auth_headeris considered, so explicitGatewayConfigcredentials are dropped wheneverallow_environment_provider_authis false. Move the configured-header branch ahead of that guard; keep inbound credentials taking precedence and add a false-flag test.🤖 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/cli/src/gateway/mod.rs` around lines 936 - 947, Update inject_provider_auth_with_env so configured_auth_header is applied before checking allow_environment_provider_auth, while preserving the early return when inbound already contains credentials. Keep environment-based authentication gated by the flag, and add a test covering configured GatewayConfig credentials when the flag is false.
🤖 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/cli/tests/coverage/shared/gateway_tests.rs`:
- Around line 1391-1477: Extend the coverage around configured provider
authentication by adding tests for empty or invalid configured auth headers from
both file configuration and environment configuration, asserting they are
rejected through the relevant API error path. Add cross-request/endpoint
isolation coverage showing credentials from one provider endpoint are cleared
and cannot carry over when the request is routed to another provider, alongside
the existing tests in
configured_auth_headers_are_provider_specific_and_precede_environment_keys and
configured_openai_auth_replaces_chatgpt_jwt.
---
Outside diff comments:
In `@crates/cli/src/gateway/mod.rs`:
- Around line 936-947: Update inject_provider_auth_with_env so
configured_auth_header is applied before checking
allow_environment_provider_auth, while preserving the early return when inbound
already contains credentials. Keep environment-based authentication gated by the
flag, and add a test covering configured GatewayConfig credentials when the flag
is false.
🪄 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: b7a9d330-c06c-4ef4-b973-e319128d728b
📒 Files selected for processing (5)
crates/cli/src/configuration/mod.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_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 (9)
**/*.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/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/gateway/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/shared/gateway_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/gateway/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/shared/gateway_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/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/gateway/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/shared/gateway_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/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/gateway/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/shared/gateway_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/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/gateway/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/shared/gateway_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/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/gateway/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/shared/gateway_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.
Files:
crates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/gateway/mod.rscrates/cli/src/configuration/mod.rscrates/cli/tests/coverage/shared/gateway_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/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/tests/coverage/shared/gateway_tests.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/cli/src/gateway/mod.rscrates/cli/src/configuration/mod.rs
🔇 Additional comments (7)
crates/cli/src/configuration/mod.rs (2)
97-103: 📐 Maintainability & Code QualityRun the mandated Rust validation before handoff.
Verify
cargo fmt --all,just test-rust,cargo clippy --workspace --all-targets -- -D warnings, anduv run pre-commit run --all-filesfor this stack.As per coding guidelines, Rust changes require these formatting, test, lint, and pre-commit checks before review or handoff.
Source: Coding guidelines
17-17: LGTM!Also applies to: 66-70, 232-250, 248-250, 862-869, 900-906, 1010-1010, 1184-1184, 1210-1245, 1512-1536, 1549-1557, 1591-1612
crates/cli/src/gateway/mod.rs (2)
103-110: 📐 Maintainability & Code QualityVerify that warning output remains user-visible.
These branches replaced unconditional
eprintln!calls with filterablelog::warn!. If logging is uninitialized or the configured sink/level excludes warnings, the previous user-facing diagnostic disappears. Keep stderr output alongside the structured event, or add a regression test proving the warning always reaches the intended sink.As per PR objectives, user-facing output is expected to remain preserved.
Also applies to: 805-809
140-141: LGTM!Also applies to: 262-266, 446-450, 723-741, 906-935, 989-990, 1128-1163
crates/cli/tests/coverage/shared/gateway_tests.rs (1)
19-19: LGTM!Also applies to: 23-27, 134-140, 169-175, 216-218, 227-229, 238-240, 257-263, 373-375, 405-407, 687-694, 1349-1358, 1374-1383, 1491-1500, 1513-1522, 1532-1540, 1657-1663, 1695-1701
crates/cli/tests/coverage/shared/server_tests.rs (1)
161-167: LGTM!Also applies to: 202-216
crates/cli/tests/coverage/shared/session_tests.rs (1)
6231-6231: LGTM!Also applies to: 6875-6875, 914-916, 2167-2169, 2439-2441, 2520-2522, 2604-2606, 3911-3913, 3989-3991, 4063-4065, 4121-4123, 4606-4608, 4735-4737, 4834-4836, 4951-4953, 5046-5048, 6879-6881, 6895-6897
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cli/src/gateway/mod.rs (1)
936-947: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward configured auth even when env auth is disabled
inject_provider_auth_with_envreturns beforeconfigured_auth_headeris considered, so explicitGatewayConfigcredentials are dropped wheneverallow_environment_provider_authis false. Move the configured-header branch ahead of that guard; keep inbound credentials taking precedence and add a false-flag test.🤖 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/cli/src/gateway/mod.rs` around lines 936 - 947, Update inject_provider_auth_with_env so configured_auth_header is applied before checking allow_environment_provider_auth, while preserving the early return when inbound already contains credentials. Keep environment-based authentication gated by the flag, and add a test covering configured GatewayConfig credentials when the flag is false.
🤖 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/cli/tests/coverage/shared/gateway_tests.rs`:
- Around line 1391-1477: Extend the coverage around configured provider
authentication by adding tests for empty or invalid configured auth headers from
both file configuration and environment configuration, asserting they are
rejected through the relevant API error path. Add cross-request/endpoint
isolation coverage showing credentials from one provider endpoint are cleared
and cannot carry over when the request is routed to another provider, alongside
the existing tests in
configured_auth_headers_are_provider_specific_and_precede_environment_keys and
configured_openai_auth_replaces_chatgpt_jwt.
---
Outside diff comments:
In `@crates/cli/src/gateway/mod.rs`:
- Around line 936-947: Update inject_provider_auth_with_env so
configured_auth_header is applied before checking
allow_environment_provider_auth, while preserving the early return when inbound
already contains credentials. Keep environment-based authentication gated by the
flag, and add a test covering configured GatewayConfig credentials when the flag
is false.
🪄 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: b7a9d330-c06c-4ef4-b973-e319128d728b
📒 Files selected for processing (5)
crates/cli/src/configuration/mod.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rs
📜 Review details
🔇 Additional comments (7)
crates/cli/src/configuration/mod.rs (2)
97-103: 📐 Maintainability & Code QualityRun the mandated Rust validation before handoff.
Verify
cargo fmt --all,just test-rust,cargo clippy --workspace --all-targets -- -D warnings, anduv run pre-commit run --all-filesfor this stack.As per coding guidelines, Rust changes require these formatting, test, lint, and pre-commit checks before review or handoff.
Source: Coding guidelines
17-17: LGTM!Also applies to: 66-70, 232-250, 248-250, 862-869, 900-906, 1010-1010, 1184-1184, 1210-1245, 1512-1536, 1549-1557, 1591-1612
crates/cli/src/gateway/mod.rs (2)
103-110: 📐 Maintainability & Code QualityVerify that warning output remains user-visible.
These branches replaced unconditional
eprintln!calls with filterablelog::warn!. If logging is uninitialized or the configured sink/level excludes warnings, the previous user-facing diagnostic disappears. Keep stderr output alongside the structured event, or add a regression test proving the warning always reaches the intended sink.As per PR objectives, user-facing output is expected to remain preserved.
Also applies to: 805-809
140-141: LGTM!Also applies to: 262-266, 446-450, 723-741, 906-935, 989-990, 1128-1163
crates/cli/tests/coverage/shared/gateway_tests.rs (1)
19-19: LGTM!Also applies to: 23-27, 134-140, 169-175, 216-218, 227-229, 238-240, 257-263, 373-375, 405-407, 687-694, 1349-1358, 1374-1383, 1491-1500, 1513-1522, 1532-1540, 1657-1663, 1695-1701
crates/cli/tests/coverage/shared/server_tests.rs (1)
161-167: LGTM!Also applies to: 202-216
crates/cli/tests/coverage/shared/session_tests.rs (1)
6231-6231: LGTM!Also applies to: 6875-6875, 914-916, 2167-2169, 2439-2441, 2520-2522, 2604-2606, 3911-3913, 3989-3991, 4063-4065, 4121-4123, 4606-4608, 4735-4737, 4834-4836, 4951-4953, 5046-5048, 6879-6881, 6895-6897
🛑 Comments failed to post (1)
crates/cli/tests/coverage/shared/gateway_tests.rs (1)
1391-1477: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add negative and credential-isolation coverage.
The new tests cover valid configured headers and JWT replacement, but not invalid/empty header rejection or clearing inherited credentials when an endpoint changes. Add file- and environment-config tests for those paths, and verify that a credential for one endpoint cannot carry over to another.
As per path instructions, tests under
crates/**/tests/**should cover changed API error paths and cross-request isolation where relevant.🤖 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/cli/tests/coverage/shared/gateway_tests.rs` around lines 1391 - 1477, Extend the coverage around configured provider authentication by adding tests for empty or invalid configured auth headers from both file configuration and environment configuration, asserting they are rejected through the relevant API error path. Add cross-request/endpoint isolation coverage showing credentials from one provider endpoint are cleared and cannot carry over when the request is routed to another provider, alongside the existing tests in configured_auth_headers_are_provider_specific_and_precede_environment_keys and configured_openai_auth_replaces_chatgpt_jwt.Source: Path instructions
|
/merge |
2 participants
Overview
Add structured operational logging throughout the core runtime and CLI, including lifecycle outcomes, recoverable degradation, final failures, and built-in plugin resource-access validation.
Details
nemo_relay.<subsystem>targets and snake_case event names at outcome-owning boundaries across core runtime, plugins, workers, observability, CLI commands, servers, sessions, agents, bootstrap, MCP, hooks, installation, and diagnostics.Where should the reviewer start?
Start with
crates/cli/tests/architecture_tests.rsfor the enforced logging contract, then reviewcrates/core/src/observability/atof.rsandcrates/cli/src/commands/mod.rsfor representative core and CLI instrumentation boundaries.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Validation
cargo check -p nemo-relay --all-featurescargo check -p nemo-relay-clijust test-rustjust test-python(528 passed)just test-node(264 passed)just test-gouv run pre-commit run --all-filescargo test -p nemo-relay-cli --lib(1,049 passed; no working-directory warnings)Summary by CodeRabbit
stderr/eprintln!output with structured warnings/errors; improved visibility for request/dispatch/gateway fallback paths and hook delivery failures.CliErrorwith consistent, stable log-kind mapping.