fix: expose effective upstream auth presence in CLI diagnostics - #660
Conversation
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
github-actions
Bot
added
size:M
WalkthroughConfiguration diagnostics now report OpenAI and Anthropic authentication presence from resolved gateway settings. Statuses can be ChangesUpstream authentication diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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: 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/tests/cli_tests.rs`:
- Around line 3307-3343: Add negative output assertions in both test sites: in
crates/cli/tests/cli_tests.rs lines 3307-3343, verify the doctor --json output
excludes “Bearer project-openai” and “Basic project-anthropic” before parsing or
consuming it; in crates/cli/tests/cli_tests.rs lines 3693-3727, add the same
exclusions for the run --dry-run output. Preserve the existing derived-label
assertions.
🪄 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: 4317c619-380a-453b-9762-c1b19e89d5fb
📒 Files selected for processing (6)
crates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.rscrates/cli/src/diagnostics/render.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rs
📜 Review details
🧰 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/diagnostics/render.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.rscrates/cli/tests/coverage/shared/doctor_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/src/diagnostics/render.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.rscrates/cli/tests/coverage/shared/doctor_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/diagnostics/render.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.rscrates/cli/tests/coverage/shared/doctor_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/diagnostics/render.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.rscrates/cli/tests/coverage/shared/doctor_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/diagnostics/render.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.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/render.rscrates/cli/src/process/launcher.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.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/diagnostics/render.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.rscrates/cli/tests/coverage/shared/doctor_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/diagnostics/render.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.rscrates/cli/tests/coverage/shared/doctor_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/diagnostics/render.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.rscrates/cli/tests/coverage/shared/doctor_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/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rs
🔇 Additional comments (5)
crates/cli/src/diagnostics/model.rs (1)
67-107: 📐 Maintainability & Code QualityRun required Rust validation.
Before merge, run
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, andjust test-rust. Attach the results.As per coding guidelines, “Any Rust change must run
just test-rust”, “Any Rust change must runcargo fmt --all”, and “Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings”.Source: Coding guidelines
crates/cli/src/diagnostics/mod.rs (1)
126-129: LGTM!Also applies to: 142-208
crates/cli/src/diagnostics/render.rs (1)
114-118: LGTM!crates/cli/src/process/launcher.rs (1)
614-625: LGTM!Also applies to: 659-665
crates/cli/tests/coverage/shared/doctor_tests.rs (1)
10-10: LGTM!Also applies to: 78-81, 257-269, 587-587, 610-611, 864-893
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
…-auth-presence-diagnostics 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/model.rs (1)
104-113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat blank configured headers as unset.
Some("")and whitespace-only header values returnConfigured. These values cannot provide effective upstream authentication. This makesdoctorandrun --dry-runreport a false configured state.Add a regression test for blank configured OpenAI and Anthropic headers.
Proposed fix
- if configured_auth_header.is_some() || env_var_is_nonempty(fallback_env_var) { + if configured_auth_header + .map(|value| !value.trim().is_empty()) + .unwrap_or(false) + || env_var_is_nonempty(fallback_env_var) + {🤖 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/model.rs` around lines 104 - 113, Update from_effective_provider_auth to treat configured_auth_header as present only when it is non-empty after trimming whitespace, while preserving the fallback environment-variable check. Add regression coverage verifying blank and whitespace-only configured OpenAI and Anthropic headers are reported as Unset.
🤖 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/model.rs`:
- Around line 104-113: Update from_effective_provider_auth to treat
configured_auth_header as present only when it is non-empty after trimming
whitespace, while preserving the fallback environment-variable check. Add
regression coverage verifying blank and whitespace-only configured OpenAI and
Anthropic headers are reported as Unset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 08739e83-7e49-439e-8e0a-cfb65ba984d4
📒 Files selected for processing (5)
crates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Rust / Test (macos-arm64)
- GitHub Check: Rust / Test (linux-arm64)
- GitHub Check: Rust / Test (windows-amd64)
- GitHub Check: Rust / Test (linux-amd64)
- GitHub Check: Rust / Test (windows-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/process/launcher.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/model.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/src/process/launcher.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/model.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/process/launcher.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/model.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/process/launcher.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/model.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/process/launcher.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/model.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/process/launcher.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.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/process/launcher.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/model.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/process/launcher.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/model.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/process/launcher.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/model.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/cli_tests.rscrates/cli/tests/coverage/shared/doctor_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/process/launcher.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/diagnostics/model.rs
🔇 Additional comments (1)
crates/cli/src/diagnostics/model.rs (1)
74-129: 📐 Maintainability & Code QualityRun the required Rust validation before handoff.
Run
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust, anduv run pre-commit run --all-files. Attach the results to the PR.As per coding guidelines, “Any Rust change must run
just test-rust,” “runcargo fmt --all,” and “runcargo clippy --workspace --all-targets -- -D warnings.”Source: Coding guidelines
|
/merge |
2 participants
Overview
Expose effective upstream auth presence in CLI diagnostics without leaking secrets.
Details
doctor --json, humandoctoroutput, andrun --dry-rununknowninstead of misleadingunsetwhen configuration resolution fails before effective auth can be determinedWhere should the reviewer start?
Start in
crates/cli/src/diagnostics/model.rsandcrates/cli/src/diagnostics/mod.rsfor the effective auth-state model and collection logic, thencrates/cli/src/process/launcher.rsfor therun --dry-runrendering. The most relevant end-to-end coverage is incrates/cli/tests/cli_tests.rs.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit