fix: add offline mode to doctor probes - #670
Conversation
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
github-actions
Bot
added
size:M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📜 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:
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Files:
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2026-08-03T19:55:03.931ZApplied to files:
🔇 Additional comments (3)
WalkthroughThe doctor command now supports ChangesOffline doctor diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DoctorCommand
participant run_doctor
participant collect_report
participant ObservabilityChecks
DoctorCommand->>run_doctor: pass DoctorProbeMode
run_doctor->>collect_report: collect report with probe mode
collect_report->>ObservabilityChecks: check configured components
ObservabilityChecks-->>collect_report: validate locally or run live probe
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/diagnostics/mod.rs`:
- Around line 1127-1135: The offline branch in the sink diagnostics flow returns
before validating the URL, transport, and transport-specific scheme. In
crates/cli/src/diagnostics/mod.rs lines 1127-1135, move the offline
skipped-probe return after those validations so invalid WebSocket schemes and
unsupported transports produce Status::Fail. In
crates/cli/tests/coverage/shared/doctor_tests.rs lines 1786-1802, add offline
coverage for both invalid cases and assert Status::Fail.
In `@crates/cli/src/diagnostics/probes.rs`:
- Around line 121-135: Update validate_grpc_endpoint and
validate_otlp_http_endpoint to reject parsed URLs whose scheme is not http or
https, while preserving their existing host and parse-error validation. In
crates/cli/tests/coverage/shared/doctor_tests.rs lines 1201-1254, add offline
diagnostics cases for unsupported endpoint schemes and assert Status::Fail for
each.
🪄 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: 6e7af94f-ef4e-4b78-9ace-f03aef3e8f71
📒 Files selected for processing (8)
crates/cli/src/commands/diagnostics.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/shared/probes_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Check / Run
🧰 Additional context used
📓 Path-based instructions (10)
**/*.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/commands/main_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
**/*.{rs,py,go,js,ts}: When observability configuration or lifecycle is exposed, keep FFI and Python, Go, and Node.js binding-native config objects and subscriber/exporter methods aligned in logical knobs and semantics.
Require every OpenTelemetry endpoint to have a type and nonblank destination; resolveheader_envvalues at activation and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Preserve correct handling of mark events, start/end events, orphan cases, and span or trajectory fields derived from intended event data.
Run affected Rust tests andjust test-rustwhen event fields change; runjust test-python,just test-go, andjust test-nodewhen binding-native configuration or lifecycle changes.
Files:
crates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/probes_tests.rscrates/cli/tests/coverage/shared/doctor_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/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/diagnostics/probes.rscrates/cli/src/diagnostics/mod.rs
🧠 Learnings (1)
📚 Learning: 2026-08-03T19:55:03.931Z
Learnt from: afourniernv
Repo: NVIDIA/NeMo-Relay PR: 558
File: crates/pii-redaction/src/rampart/mod.rs:265-274
Timestamp: 2026-08-03T19:55:03.931Z
Learning: In NeMo Relay first-party plugin registration helpers, treat the documented duplicate-registration `PluginError::RegistrationFailed` result from `register_plugin` as success when registration is intended to be idempotent. Do not locally reclassify this as `PluginError::Conflict`; changing the classification requires a core-wide review of the public API and FFI behavior.
Applied to files:
crates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/root.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
🔇 Additional comments (1)
crates/cli/src/diagnostics/mod.rs (1)
49-63: 📐 Maintainability & Code QualityProvide the required Rust validation results.
Run
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust, anduv run pre-commit run --all-filesbefore handoff.As per coding guidelines, any Rust change requires these validation steps.
Source: Coding guidelines
Signed-off-by: mnajafian-nv <mnajafian@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 (1)
crates/cli/src/diagnostics/mod.rs (1)
1117-1133: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject blank and duplicate static ATOF headers before the offline skip.
endpoint_headersaccepts blank static values and case-insensitive duplicate names withinheaders. For example,Authorizationandauthorizationboth pass. An offline check then reportsInfoinstead of rejecting invalid configuration.Validate each static and resolved environment value with
HeaderValue::from_bytes. Reject whitespace-only values. Reject duplicate names whenHashSet::insertreturnsfalse. Add offline regression cases for blank and duplicate static headers.🤖 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/diagnostics/mod.rs` around lines 1117 - 1133, Update the ATOF header validation used by endpoint_headers to validate both static values and resolved environment values with HeaderValue::from_bytes, reject whitespace-only values, and reject case-insensitive duplicate names when HashSet::insert returns false. Ensure these checks run before the offline skip so invalid configurations return Status::Fail, and add offline regression cases covering blank and duplicate static headers.Source: Coding guidelines
🤖 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/diagnostics/mod.rs`:
- Around line 1117-1133: Update the ATOF header validation used by
endpoint_headers to validate both static values and resolved environment values
with HeaderValue::from_bytes, reject whitespace-only values, and reject
case-insensitive duplicate names when HashSet::insert returns false. Ensure
these checks run before the offline skip so invalid configurations return
Status::Fail, and add offline regression cases covering blank and duplicate
static headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ee1b1ace-1d69-4f72-953e-e0ad80cdf47d
📒 Files selected for processing (4)
crates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/shared/probes_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Check / Run
🧰 Additional context used
📓 Path-based instructions (10)
**/*.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/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.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/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
**/*.{rs,py,go,js,ts}: When observability configuration or lifecycle is exposed, keep FFI and Python, Go, and Node.js binding-native config objects and subscriber/exporter methods aligned in logical knobs and semantics.
Require every OpenTelemetry endpoint to have a type and nonblank destination; resolveheader_envvalues at activation and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Preserve correct handling of mark events, start/end events, orphan cases, and span or trajectory fields derived from intended event data.
Run affected Rust tests andjust test-rustwhen event fields change; runjust test-python,just test-go, andjust test-nodewhen binding-native configuration or lifecycle changes.
Files:
crates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/shared/probes_tests.rscrates/cli/tests/coverage/shared/doctor_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/diagnostics/probes.rscrates/cli/src/diagnostics/mod.rs
🧠 Learnings (1)
📚 Learning: 2026-08-03T19:55:03.931Z
Learnt from: afourniernv
Repo: NVIDIA/NeMo-Relay PR: 558
File: crates/pii-redaction/src/rampart/mod.rs:265-274
Timestamp: 2026-08-03T19:55:03.931Z
Learning: In NeMo Relay first-party plugin registration helpers, treat the documented duplicate-registration `PluginError::RegistrationFailed` result from `register_plugin` as success when registration is intended to be idempotent. Do not locally reclassify this as `PluginError::Conflict`; changing the classification requires a core-wide review of the public API and FFI behavior.
Applied to files:
crates/cli/tests/coverage/shared/probes_tests.rscrates/cli/src/diagnostics/probes.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rs
🔇 Additional comments (5)
crates/cli/src/diagnostics/mod.rs (2)
74-76: 📐 Maintainability & Code QualityProvide Rust validation results.
Run
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, andjust test-rust. Runuv run pre-commit run --all-filesbefore handoff.Source: Coding guidelines
151-151: LGTM!Also applies to: 1171-1202, 1514-1518
crates/cli/src/diagnostics/probes.rs (1)
121-148: LGTM!crates/cli/tests/coverage/shared/doctor_tests.rs (1)
1256-1289: LGTM!Also applies to: 1839-1870
crates/cli/tests/coverage/shared/probes_tests.rs (1)
21-49: LGTM!
willkill07
left a comment
willkill07
left a comment
There was a problem hiding this comment.
We have a regression in validating other endpoint fields in --offline mode. We need to fix that.
Otherwise, just minor nits.
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/diagnostics/mod.rs`:
- Around line 1258-1259: Add a diagnostic test around probe_atof_endpoint that
sets an invalid header_env environment variable, invokes the probe, and asserts
it returns Status::Fail with the “header_env.<key> invalid” diagnostic. Ensure
this validation failure is asserted before any live probe execution.
🪄 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: d3fc7192-d626-4623-ad01-86978565746d
📒 Files selected for processing (4)
crates/adaptive/src/response_cache/store.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_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 (14)
**/*.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/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rscrates/adaptive/src/response_cache/store.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rscrates/adaptive/src/response_cache/store.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/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rscrates/adaptive/src/response_cache/store.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/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rscrates/adaptive/src/response_cache/store.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/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rscrates/adaptive/src/response_cache/store.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/commands/diagnostics.rscrates/cli/src/diagnostics/mod.rscrates/adaptive/src/response_cache/store.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/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rscrates/adaptive/src/response_cache/store.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
**/*.{rs,py,go,js,ts}: When observability configuration or lifecycle is exposed, keep FFI and Python, Go, and Node.js binding-native config objects and subscriber/exporter methods aligned in logical knobs and semantics.
Require every OpenTelemetry endpoint to have a type and nonblank destination; resolveheader_envvalues at activation and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Preserve correct handling of mark events, start/end events, orphan cases, and span or trajectory fields derived from intended event data.
Run affected Rust tests andjust test-rustwhen event fields change; runjust test-python,just test-go, andjust test-nodewhen binding-native configuration or lifecycle changes.
Files:
crates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rscrates/adaptive/src/response_cache/store.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rscrates/adaptive/src/response_cache/store.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/doctor_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/adaptive/src/response_cache/store.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/response_cache/store.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/response_cache/store.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/response_cache/store.rs
🧠 Learnings (1)
📚 Learning: 2026-08-03T19:55:03.931Z
Learnt from: afourniernv
Repo: NVIDIA/NeMo-Relay PR: 558
File: crates/pii-redaction/src/rampart/mod.rs:265-274
Timestamp: 2026-08-03T19:55:03.931Z
Learning: In NeMo Relay first-party plugin registration helpers, treat the documented duplicate-registration `PluginError::RegistrationFailed` result from `register_plugin` as success when registration is intended to be idempotent. Do not locally reclassify this as `PluginError::Conflict`; changing the classification requires a core-wide review of the public API and FFI behavior.
Applied to files:
crates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/mod.rscrates/adaptive/src/response_cache/store.rs
🔇 Additional comments (6)
crates/cli/src/commands/diagnostics.rs (1)
24-28: LGTM!crates/adaptive/src/response_cache/store.rs (3)
448-458: LGTM!Also applies to: 480-480, 502-513
448-458: 📐 Maintainability & Code QualityRun the required adaptive validation matrix.
This change modifies
crates/adaptive. Provide successful results forcargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust,just test-python,just test-go,just test-node, anduv run pre-commit run --all-files. As per coding guidelines,crates/adaptivechanges require full validation across Rust, Python, Go, and Node.js.Source: Coding guidelines
515-530: 🎯 Functional CorrectnessNo change required.
crates/cli/Cargo.tomlunconditionally enablesnemo-relay-adaptivewithredis-backend, so the"redis client"assertion is valid for this CLI test.> Likely an incorrect or invalid review comment.crates/cli/tests/coverage/shared/doctor_tests.rs (1)
1844-1894: LGTM!Also applies to: 1955-1986
crates/cli/src/diagnostics/mod.rs (1)
731-733: 📐 Maintainability & Code QualityKeep the
letchain. The repository uses Rust 1.96.1, which supports this syntax.> Likely an incorrect or invalid review comment.
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
#### Overview Prevents Python tool execution from queuing a completion callback after its owning asyncio event loop has closed during cancellation. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Replace the generic PyO3 completion bridge for managed tool execution with a loop-aware bridge. - Cancel the Rust task when its Python future is cancelled. - Skip completion scheduling when the owning event loop is closed. #### Where should the reviewer start? Start with `crates/python/src/py_api/mod.rs`, especially `safe_future_into_py` and its cancellation/completion callbacks. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to #670 ## Summary by CodeRabbit * **Bug Fixes** * Improved cancellation handling for Python asynchronous operations. * Prevented callbacks from running against cancelled or closed event loops. * Ensured operation results and errors are delivered safely through the Python event loop. * Tool execution now responds more reliably when interrupted or cancelled. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Maryam Najafian (https://github.com/mnajafian-nv) URL: #676
|
/merge |
2 participants
Overview
Add an offline mode to
nemo-relay doctorso observability diagnostics can validate configured exporters without requiring live network reachability. Live probe output now also makes it explicit when a check is only confirming HTTP reachability or gRPC TCP reachability.Details
--offlinetonemo-relay doctorand thread an explicit probe mode through the doctor collection pathValidation:
just test-rustcargo fmt --allcargo clippy --workspace --all-targets -- -D warningsuv run pre-commit run --files crates/cli/src/commands/diagnostics.rs crates/cli/src/commands/mod.rs crates/cli/src/commands/root.rs crates/cli/src/diagnostics/mod.rs crates/cli/src/diagnostics/probes.rs crates/cli/tests/coverage/commands/main_tests.rs crates/cli/tests/coverage/shared/doctor_tests.rs crates/cli/tests/coverage/shared/probes_tests.rscargo test -p nemo-relay-cli doctor_accepts_offline_flag -- --nocapturecargo test -p nemo-relay-cli diagnostics::tests -- --nocapturecargo test -p nemo-relay-cli diagnostics::probes::tcp_tests -- --nocaptureWhere should the reviewer start?
Start in
crates/cli/src/diagnostics/mod.rs, especiallyobservability_http_exporter_checks()andprobe_atof_stream_sink(). The most relevant regression coverage is incrates/cli/tests/coverage/shared/doctor_tests.rs.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
--offlineoption to thedoctorcommand.Bug Fixes
Tests