fix: report logging config errors in doctor - #662
Conversation
WalkthroughDoctor now preserves logging configuration fallback errors, passes them through CLI dispatch, and reports them in configuration resolution results. Tests cover missing, invalid, and valid explicit logging configurations. ChangesDoctor logging fallback diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant LoggingConfiguration
participant CommandDispatch
participant Doctor
participant DoctorReport
CLI->>LoggingConfiguration: configure logging
LoggingConfiguration-->>CLI: runtime and optional fallback error
CLI->>CommandDispatch: execute command with fallback error
CommandDispatch->>Doctor: run diagnostics with fallback error
Doctor->>DoctorReport: update configuration resolution
DoctorReport-->>CLI: report and exit result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Eric Evans <194135482+ericevans-nv@users.noreply.github.com>
2f87398 to
2d56e1a
Compare
willkill07
left a comment
There was a problem hiding this comment.
One nit on datatype -- rather than an arbitrary tuple, a struct with named fields promotes readability.
Conditionally approving
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/commands/mod.rs (1)
87-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not suppress logging failures for plugin Doctor.
Lines 87-90 apply the fallback to every
Command::Doctor. However,crates/cli/src/commands/diagnostics.rsreturns early forcommand.pluginand does not consumelogging_fallback_error. Therefore,doctor --plugincan succeed after an invalid explicit logging configuration.Restrict the fallback to normal Doctor runs, or report the fallback error in the plugin Doctor path and return failure. Add an integration test for this command 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/cli/src/commands/mod.rs` around lines 87 - 90, Restrict the logging fallback in the command dispatch match to normal Doctor runs, excluding `Command::Doctor` requests with a plugin, so invalid explicit logging configuration is not suppressed. Alternatively, update the plugin path in `diagnostics.rs` to consume `logging_fallback_error` and return failure; add an integration test covering `doctor --plugin` with invalid logging configuration.
🤖 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/commands/mod.rs`:
- Around line 87-90: Restrict the logging fallback in the command dispatch match
to normal Doctor runs, excluding `Command::Doctor` requests with a plugin, so
invalid explicit logging configuration is not suppressed. Alternatively, update
the plugin path in `diagnostics.rs` to consume `logging_fallback_error` and
return failure; add an integration test covering `doctor --plugin` with invalid
logging configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a52c6866-0f3c-47de-9099-6c5404f27fc5
📒 Files selected for processing (5)
crates/cli/src/commands/diagnostics.rscrates/cli/src/commands/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/commands/main_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Rust / Test (linux-arm64)
- GitHub Check: Rust / Test (windows-arm64)
- GitHub Check: Rust / Test (windows-amd64)
- GitHub Check: Rust / Test (linux-amd64)
- GitHub Check: Rust / Test (macos-arm64)
🧰 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/src/commands/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/cli_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/src/commands/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/cli_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/src/commands/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/cli_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/src/commands/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/cli_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/src/commands/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/cli_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/diagnostics/mod.rscrates/cli/src/commands/diagnostics.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/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/cli_tests.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
**/*.{rs,py,go,js,ts}: When observability configuration or lifecycle is exposed, keep FFI and Python, Go, and Node.js binding-native config objects and subscriber/exporter methods aligned in logical knobs and semantics.
Require every OpenTelemetry endpoint to have a type and nonblank destination; resolveheader_envvalues at activation and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Preserve correct handling of mark events, start/end events, orphan cases, and span or trajectory fields derived from intended event data.
Run affected Rust tests andjust test-rustwhen event fields change; runjust test-python,just test-go, andjust test-nodewhen binding-native configuration or lifecycle changes.
Files:
crates/cli/src/commands/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/cli_tests.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/cli/src/commands/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/cli_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/commands/main_tests.rscrates/cli/tests/cli_tests.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/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/cli_tests.rs
🔇 Additional comments (5)
crates/cli/src/commands/mod.rs (2)
58-64: 📐 Maintainability & Code QualityRun the required Rust validation commands.
The supplied validation summary does not confirm all required commands. Run
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust, anduv run pre-commit run --all-files. Attach the results before handoff.As per coding guidelines, “Any Rust change must run
just test-rust” and Rust changes must run the specified formatting, Clippy, and pre-commit commands.Source: Coding guidelines
113-123: LGTM!Also applies to: 159-181, 233-233
crates/cli/src/diagnostics/mod.rs (1)
1367-1388: LGTM!crates/cli/tests/cli_tests.rs (1)
3269-3390: LGTM!crates/cli/tests/coverage/commands/main_tests.rs (1)
504-520: LGTM!
Signed-off-by: Eric Evans <194135482+ericevans-nv@users.noreply.github.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/commands/mod.rs (1)
162-166: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winForward the fallback error for
Doctor --plugin.The
command.pluginbranch returns beforerun_doctor, so it dropslogging_fallback_errorfrom plugin Doctor output. Handle the fallback before this early return or include it in the plugin report.🤖 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/commands/mod.rs` around lines 162 - 166, Update run_command so the Doctor --plugin branch preserves and forwards logging_fallback_error before returning, either by handling the fallback before the early return or passing it into the plugin report; keep the existing non-plugin run_doctor behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/cli/src/commands/mod.rs`:
- Around line 162-166: Update run_command so the Doctor --plugin branch
preserves and forwards logging_fallback_error before returning, either by
handling the fallback before the early return or passing it into the plugin
report; keep the existing non-plugin run_doctor behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0e716e07-66f5-4162-851a-2c0f46fdde5f
📒 Files selected for processing (1)
crates/cli/src/commands/mod.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/src/commands/mod.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/src/commands/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/src/commands/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/src/commands/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/src/commands/mod.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.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/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/src/commands/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/src/commands/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/src/commands/mod.rs
🔇 Additional comments (2)
crates/cli/src/commands/mod.rs (2)
70-73: LGTM!Also applies to: 94-105, 116-126, 236-236
56-61: 📐 Maintainability & Code QualityRun the required Rust checks before handoff.
The checks did not complete within the execution limit, so their results are unknown. Run each command separately and report the results.
|
/merge |
Overview
Report explicit logging configuration failures through
nemo-relay doctorinstead of silently falling back and reporting that all checks passed.Details
Resolutioncheck for both human and JSON output.Validation:
cargo test -p nemo-relay-cli --test cli_tests cli_doctor_ -- --nocapture: 12 passed.cargo test -p nemo-relay-cli --lib run_command_dispatches_safe_plugin_and_install_paths -- --nocapture: 1 passed.just test-all: Rust stopped with 1068 passed and 86 unrelated failures beginning in existing core tests that use unsupported observability config version 1; push proceeded with an explicit waiver.Where should the reviewer start?
Start with
crates/cli/src/diagnostics/mod.rs, where the preserved logging error is merged into the existing configuration resolution result, then review the CLI regression cases incrates/cli/tests/cli_tests.rs.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Bug Fixes
Tests