fix: reject invalid upstream shared config keys - #657
Conversation
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
github-actions
Bot
added
size:S
WalkthroughConfiguration parsing now rejects unknown upstream fields and exposes shared shape validation. Layer diagnostics validate parsed TOML structure before plugin-specific checks. Tests cover invalid upstream and gateway configuration shapes. ChangesConfiguration validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant layer_status
participant toml_layer_status
participant validate_shared_config_shape
participant ConfigLayer
layer_status->>toml_layer_status: Read and parse configuration TOML
toml_layer_status-->>layer_status: Return TOML status
layer_status->>validate_shared_config_shape: Validate shared configuration shape
validate_shared_config_shape-->>layer_status: Return validation result
layer_status->>ConfigLayer: Report active or inactive layer status
🚥 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`:
- Line 360: Expand the doctor diagnostics tests around plugin_layer_status and
toml_layer_status to include a valid plugins.toml fixture containing
plugin-specific structure, and assert it is accepted without
validate_shared_config_shape rejecting it. Keep the existing invalid-TOML
coverage through resolve_server_config, while adding this regression case to
verify the plugin-layer boundary uses TOML-only validation.
In `@crates/cli/tests/coverage/shared/doctor_tests.rs`:
- Around line 417-430: Update the invalid-shape case in the layer_status test to
also assert that invalid_shape_layer.active is false, while preserving the
existing failure status and detail-message 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: 10974770-fe51-4051-a73a-3b929098ce29
📒 Files selected for processing (4)
crates/cli/src/configuration/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/config_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/tests/coverage/shared/doctor_tests.rscrates/cli/src/configuration/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/config_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/doctor_tests.rscrates/cli/src/configuration/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/config_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/doctor_tests.rscrates/cli/src/configuration/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/config_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/doctor_tests.rscrates/cli/src/configuration/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/config_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/doctor_tests.rscrates/cli/src/configuration/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/config_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/doctor_tests.rscrates/cli/src/configuration/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/config_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/tests/coverage/shared/doctor_tests.rscrates/cli/src/configuration/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/config_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/tests/coverage/shared/doctor_tests.rscrates/cli/src/configuration/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/config_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/doctor_tests.rscrates/cli/tests/coverage/shared/config_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/configuration/mod.rscrates/cli/src/diagnostics/mod.rs
🔇 Additional comments (3)
crates/cli/src/configuration/mod.rs (1)
68-68: LGTM!Also applies to: 1218-1235
crates/cli/src/diagnostics/mod.rs (1)
277-308: LGTM!crates/cli/tests/coverage/shared/config_tests.rs (1)
3432-3463: LGTM!
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/tests/coverage/shared/doctor_tests.rs (1)
1019-1038: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the request path for the bare endpoint case.
The current assertion checks
Check.details, which is formatted from the URL passed toprobe_otlp_http_named. It does not prove that the server receivedGET /v1/traces. Capture the request withread_headersand assert the request line, as the explicit-root case already does.Proposed test change
let accept = std::thread::spawn(move || { let mut stream = accept_bounded(&listener); - let _ = read_headers(&mut stream); + let request = read_headers(&mut stream); stream .write_all(b"HTTP/1.1 405 Method Not Allowed\r\nContent-Length: 0\r\n\r\n") .unwrap(); + request }); ... assert!(checks[0].details.contains("/v1/traces (HTTP 405)")); - accept.join().unwrap(); + let request = accept.join().unwrap(); + assert!(request.starts_with("GET /v1/traces HTTP/1.1"));As per path instructions, tests should cover the behavior promised by the changed API surface, including 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/doctor_tests.rs` around lines 1019 - 1038, Update the bare-endpoint test around observability_http_exporter_checks to capture the request line returned by read_headers, then assert it is GET /v1/traces rather than relying on checks[0].details. Preserve the existing HTTP 405 response and status/detail assertions, and follow the explicit-root test’s request-path assertion pattern.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/tests/coverage/shared/doctor_tests.rs`:
- Around line 1019-1038: Update the bare-endpoint test around
observability_http_exporter_checks to capture the request line returned by
read_headers, then assert it is GET /v1/traces rather than relying on
checks[0].details. Preserve the existing HTTP 405 response and status/detail
assertions, and follow the explicit-root test’s request-path assertion pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0529df37-5f13-4c12-8934-06091d1abe89
📒 Files selected for processing (2)
crates/cli/src/diagnostics/mod.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/mod.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/mod.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/mod.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/mod.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/mod.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/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/diagnostics/mod.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/mod.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/mod.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/coverage/shared/doctor_tests.rs
🔇 Additional comments (4)
crates/cli/src/diagnostics/mod.rs (2)
361-361: 🗄️ Data Integrity & IntegrationVerify the valid
plugins.tomlboundary case remains covered.
plugin_layer_statusnow uses TOML-only validation. A validplugins.tomlfile with plugin-specific structure must not be rejected byvalidate_shared_config_shape. Confirm thatdoctor_tests.rsincludes this fixture and asserts that the plugin layer remains accepted.As per path instructions, tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
#!/bin/bash set -euo pipefail rg -n -C 12 \ 'plugin_layer_status|plugins\.toml|validate_shared_config_shape|resolve_server_config' \ crates/cli/src/diagnostics/mod.rs \ crates/cli/tests/coverage/shared/doctor_tests.rsSource: Path instructions
28-28: 📐 Maintainability & Code QualityRun and report the required validation checks separately.
Execute
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust, anduv run pre-commit run --all-files. Record each result.crates/cli/tests/coverage/shared/doctor_tests.rs (2)
417-432: LGTM!
999-1017: LGTM!Also applies to: 1040-1080, 1682-1746
|
/merge |
2 participants
Overview
Reject typoed or wrongly nested shared
upstreamconfig keys and make CLI doctor fail those invalid shared-config files.Details
upstreamconfig so misspelled keys such asopenai_baseurland nested tables such as[upstream.openai]fail config resolution instead of being silently ignoredplugins.tomlon the TOML-only validation path so this fix does not change plugin config behaviorcargo test -p nemo-relay-cli malformed_shared_config_reports_contextcargo test -p nemo-relay-cli layer_status_reports_missing_valid_invalid_and_non_directory_pathscargo test -p nemo-relay-cli plugin_layer_status_marks_a_discovered_plugins_toml_as_contributingcargo fmt --allcargo clippy --workspace --all-targets -- -D warningsjust test-rustuv run pre-commit run --files crates/cli/src/configuration/mod.rs crates/cli/src/diagnostics/mod.rs crates/cli/tests/coverage/shared/config_tests.rs crates/cli/tests/coverage/shared/doctor_tests.rsuv run pre-commit run --all-filesWhere should the reviewer start?
Start in
crates/cli/src/configuration/mod.rsat the shared file-config parsing path, thencrates/cli/src/diagnostics/mod.rsatlayer_status(). The new regressions incrates/cli/tests/coverage/shared/config_tests.rsandcrates/cli/tests/coverage/shared/doctor_tests.rscover the user-visible contract.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Bug Fixes
Tests