fix!: streamline plugin configuration#346
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:M
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (8)**/*.rs📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Files:
**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,go,ts,js,mjs,cjs,jsx,tsx,c,h}📄 CodeRabbit inference engine (AGENTS.md)
Files:
crates/**/*.{rs,py,ts,js,c,h}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,go,js,ts,py,html,md,mdx,toml,yml,yaml,c,h}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
crates/**/src/**/*.rs📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Files:
**⚙️ CodeRabbit configuration file
Files:
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}⚙️ CodeRabbit configuration file
Files:
🛑 Comments failed to post (2)
🔇 Additional comments (9)
WalkthroughThis PR moves plugin configuration to ChangesPlugin configuration source change
Estimated code review effort: 3 (Moderate) | ~30 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
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/setup/model.rs (1)
160-181: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale
[plugins]blocks aren't stripped during merge, causing a load-time break.
write_or_mergeno longer touches any pre-existing[plugins]table inexisting— it only merges the target agent's block and writesexistingback verbatim. Per the config resolution contract (crates/cli/src/config.rs,load_shared_config), any top-levelpluginskey inconfig.tomlnow causes a hard error:"plugin configuration in {} is no longer supported; move it to plugins.toml".So a user with a pre-PR
config.tomlcontaining[plugins]who runsnemo-relay setupin merge mode (Some(agent)) will get a merged file that still contains[plugins], and the very nextrun/daemon invocation will fail to load that config. The merge path should actively strip any legacy[plugins]table fromexistingbefore writing it back, to make the breaking change self-healing during the natural setup flow.🔧 Proposed fix
let existing_raw = std::fs::read_to_string(path)?; let mut existing: DocumentMut = existing_raw .parse() .map_err(|err| CliError::Config(format!("could not parse existing config: {err}")))?; let agent_key = agent_key_and_command(agent).0; + // Strip any legacy [plugins] table left over from before plugin config moved to + // plugins.toml, so the merged file doesn't immediately fail to load. + existing.remove("plugins"); merge_agents_entry(&mut existing, doc, agent_key); std::fs::write(path, existing.to_string())?;🤖 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/setup/model.rs` around lines 160 - 181, write_or_merge currently merges the agent block but preserves any legacy top-level [plugins] table in existing, which can leave config.toml in a state that load_shared_config rejects. Update write_or_merge to remove or strip the pre-existing plugins section from the parsed DocumentMut before merge_agents_entry and before writing back, so merge mode self-heals old configs while still merging the target agent identified by agent_key_and_command.
♻️ Duplicate comments (1)
crates/cli/tests/coverage/setup_tests.rs (1)
449-472: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing regression coverage for legacy
[plugins]migration during merge.This test seeds a config without
[plugins]at all, so it can't catch the gap flagged oncrates/cli/src/setup/model.rs(write_or_merge, lines 160-181): if the existing file already has a[plugins]table from before this PR, merge mode currently leaves it in place, which then fails to load. Consider adding a case that seeds[plugins]in the pre-existing file and asserts it's stripped afterwrite_or_merge.🤖 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/setup_tests.rs` around lines 449 - 472, Add regression coverage in the coverage test for the legacy plugins migration path handled by write_or_merge in setup/model.rs. The current test only covers a config with no [plugins] table, so it misses the case where an existing file already contains [plugins] and merge mode leaves it behind. Update or add a test around write_or_merge_recovers_from_non_table_agents_value that seeds the temp config with a [plugins] table, runs write_or_merge, and asserts the merged output no longer contains [plugins] while still preserving the expected agents data.
🤖 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/config.rs`:
- Line 822: The plugin config lookup in `load_plugin_toml_config` currently
treats an explicit `plugin_config_path` like an optional candidate, so a missing
override can be silently ignored. Update the `plugin_toml` loading path in
`config.rs` to distinguish explicit overrides from implicit discovery: when
`explicit` is set, require `plugin_config_path` to exist and return an error if
it does not, while keeping the fallback candidate search behavior only for
non-explicit discovery. Use the existing `load_plugin_toml_config` flow and
related plugin config parsing symbols to wire the required-path check in before
continuing with plugin activation.
---
Outside diff comments:
In `@crates/cli/src/setup/model.rs`:
- Around line 160-181: write_or_merge currently merges the agent block but
preserves any legacy top-level [plugins] table in existing, which can leave
config.toml in a state that load_shared_config rejects. Update write_or_merge to
remove or strip the pre-existing plugins section from the parsed DocumentMut
before merge_agents_entry and before writing back, so merge mode self-heals old
configs while still merging the target agent identified by
agent_key_and_command.
---
Duplicate comments:
In `@crates/cli/tests/coverage/setup_tests.rs`:
- Around line 449-472: Add regression coverage in the coverage test for the
legacy plugins migration path handled by write_or_merge in setup/model.rs. The
current test only covers a config with no [plugins] table, so it misses the case
where an existing file already contains [plugins] and merge mode leaves it
behind. Update or add a test around
write_or_merge_recovers_from_non_table_agents_value that seeds the temp config
with a [plugins] table, runs write_or_merge, and asserts the merged output no
longer contains [plugins] while still preserving the expected agents data.
🪄 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: c5c59540-2106-44e9-8c85-3f24a879164d
📒 Files selected for processing (12)
crates/cli/src/config.rscrates/cli/src/installer.rscrates/cli/src/launcher.rscrates/cli/src/setup/model.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/coverage/setup_tests.rsdocs/build-plugins/plugin-configuration-files.mdxdocs/nemo-relay-cli/basic-usage.mdxintegrations/coding-agents/README.md
💤 Files with no reviewable changes (3)
- docs/nemo-relay-cli/basic-usage.mdx
- integrations/coding-agents/README.md
- crates/cli/src/installer.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 (22)
{docs/**,README.md,CONTRIBUTING.md}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
{docs/**,README.md,CONTRIBUTING.md}: For docs-only changes, run targeted checks only if commands, package names, or examples changed. Usejust docsfor docs-site builds andjust docs-linkcheckwhen links changed
Run docs site build withjust docs
Files:
docs/build-plugins/plugin-configuration-files.mdx
{docs/**,README.md,CONTRIBUTING.md,**/*.md}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Run docs link validation with
just docs-linkcheckwhen links change
Files:
docs/build-plugins/plugin-configuration-files.mdx
{docs/**,README.md}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Verify README and docs entry points still match current package names and paths for large or public-facing changes
Files:
docs/build-plugins/plugin-configuration-files.mdx
{docs/**,examples/**,README.md}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Verify examples still run with documented commands for large or public-facing changes
Files:
docs/build-plugins/plugin-configuration-files.mdx
{docs/**,README.md,**/Cargo.toml,**/package.json,**/*.md}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Ensure renamed public surfaces are reflected consistently in manifests and docs for large or public-facing changes
Files:
docs/build-plugins/plugin-configuration-files.mdx
**/*.{md,mdx,py,sh,yaml,yml,toml,json}
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Keep package names, repo references, and build commands current
Files:
docs/build-plugins/plugin-configuration-files.mdx
**/*.mdx
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
In MDX files, top-of-file comments must use JSX comment delimiters: {/* to open and */} to close. Do not use HTML comments for MDX SPDX headers.
MDX top-of-file SPDX comments must use {/* ... */} delimiters instead of HTML comment delimiters (Must-Fix)
Files:
docs/build-plugins/plugin-configuration-files.mdx
**/*.{html,md,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Include SPDX license header in HTML and Markdown files using HTML comment syntax
Files:
docs/build-plugins/plugin-configuration-files.mdx
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update embedded documentation snippets, patch docs, and binding-support notes if examples or supported bindings changed
Files:
docs/build-plugins/plugin-configuration-files.mdx
docs/**
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run
just docsor./scripts/build-docs.sh htmlto regenerate ignored Fern API reference pages before validation for documentation site changes
Files:
docs/build-plugins/plugin-configuration-files.mdx
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
This file provides guidance to agents, including Claude Code and OpenAI Codex, when working in this repository.
Project Overview
NeMo Relay is a multi-language agent runtime framework for execution scopes, lifecycle events, middleware, plugins, and observability around tool and LLM calls. The core runtime is Rust. Primary supported bindings are Rust, Python, and Node.js. Go and the raw C FFI are experimental and source-first.
The shared runtime model is:
- Scope stacks decide where work belongs and which scope-local behavior is visible.
- Middleware registries decide what guardrails and intercepts run around managed calls.
- Plugins install reusable runtime behavior from configuration.
- Events record runtime behavior in ATOF form.
- Subscribers and exporters consume events in-process or export them to ATIF, OpenTelemetry, OpenInference, or other backends.
Repository Structure
The repository layout separates the Rust runtime, language bindings,
documentation, integrations, and agent-facing skills.crates/ core/ # Rust core runtime crate, published as nemo-relay adaptive/ # Adaptive runtime primitives and plugin components python/ # PyO3 native extension for the Python package ffi/ # Raw C ABI layer used by downstream bindings such as Go node/ # NAPI Node.js binding and JavaScript/TypeScript entry points python/ nemo_relay/ # Python wrapper package: scopes, tools, LLM, middleware, typed helpers, plugins, adaptive helpers tests/ # Python tests go/ nemo_relay/ # Experimental Go CGo binding and tests fern/ # Fern documentation site scripts/ # Stable wrappers and helper scripts; build/test/docs entry points live in justfile skills/ # Published Codex/agent skills for NeMo Relay usage patternsPrerequisites
Insta...
Files:
docs/build-plugins/plugin-configuration-files.mdxcrates/cli/src/launcher.rscrates/cli/src/setup/model.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/setup_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/src/config.rs
{docs/**,README.md,CONTRIBUTING.md,RELEASING.md,SECURITY.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,RELEASING.md,SECURITY.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency across language bindings.
Flag stale examples, missing SPDX headers where required, and instructions that no longer match CI or pre-commit behavior.
Files:
docs/build-plugins/plugin-configuration-files.mdx
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Use
snake_casenaming convention for Rust identifiers (e.g.,nemo_relay_tool_call)
**/*.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 work
**/*.rs: Runcargo fmt --allto format all Rust code
Runcargo clippy --workspace --all-targets -- -D warningsto enforce all clippy lints as errors
**/*.rs: Runcargo fmt --allwhen Rust files changed as part of WebAssembly work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files changed as part of WebAssembly work
**/*.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
Run Rust formatting withcargo fmt --all
Run Rust linting withcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Usecargo fmtfor Rust code formatting
Runcargo clippy -- -D warningsto lint Rust code and treat all warnings as errors
Use Rust snake_case naming convention for Rust identifiers
Include SPDX license header in all Rust source files using double-slash comment syntax
Validate Rust code withuv run pre-commit run --all-filesto enforce cargo fmt formatting check, cargo clippy lints, and cargo deny aud...
Files:
crates/cli/src/launcher.rscrates/cli/src/setup/model.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/setup_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/src/config.rs
**/{Cargo.toml,**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Maintain consistency between Rust package names in
Cargo.tomland their actual usage across the codebase
Files:
crates/cli/src/launcher.rscrates/cli/src/setup/model.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/setup_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/src/config.rs
**/*.{h,hpp,c,cpp,rs}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Ensure FFI header and library naming follows consistent conventions across platform-specific builds
Files:
crates/cli/src/launcher.rscrates/cli/src/setup/model.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/setup_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/src/config.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (.agents/skills/rename-surfaces/SKILL.md)
Update Rust crate names and module prefixes during coordinated rename operations
Files:
crates/cli/src/launcher.rscrates/cli/src/setup/model.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/setup_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/src/config.rs
**/*.{rs,py,js,ts,tsx,jsx,go,sh,toml,yaml,yml,md}
📄 CodeRabbit inference engine (AGENTS.md)
Keep SPDX headers on source, docs, scripts, and configuration files. The project is Apache-2.0.
Files:
crates/cli/src/launcher.rscrates/cli/src/setup/model.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/setup_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/src/config.rs
**/*.{rs,py,go,js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions: Rust and Python use
snake_case, C FFI exports prefixednemo_relay_, Go usesPascalCasefor public APIs, Node.js usescamelCase.
Files:
crates/cli/src/launcher.rscrates/cli/src/setup/model.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/setup_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/src/config.rs
crates/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
crates/**/*.rs: Keep async behavior on the existing tokio-based model. Bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
UseJson = serde_json::Valuein Rust-facing runtime APIs for JSON payload handling.
Files:
crates/cli/src/launcher.rscrates/cli/src/setup/model.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/setup_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/src/config.rs
{crates/adaptive/**/*.rs,**/*test*.{rs,py,go,ts,js},**/*adaptive*test*.{rs,py,go,ts,js},docs/plugins/adaptive/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Maintain documented and tested validation and report behavior for adaptive surfaces
Files:
crates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/setup_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_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/installer_tests.rscrates/cli/tests/coverage/setup_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_tests.rs
**/*config*.{rs,ts,py,go,js,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Ensure dynamic config shape still matches the documented canonical model
Files:
crates/cli/tests/coverage/config_tests.rscrates/cli/src/config.rs
🔇 Additional comments (8)
crates/cli/src/setup/model.rs (1)
116-118: LGTM!Also applies to: 185-206
crates/cli/tests/cli_tests.rs (1)
146-152: LGTM!Also applies to: 1489-1562, 1624-1663
crates/cli/tests/coverage/config_tests.rs (1)
238-444: LGTM!Also applies to: 1145-1195, 1198-1283, 1636-1696, 1737-1779
crates/cli/tests/coverage/installer_tests.rs (1)
92-118: LGTM!crates/cli/tests/coverage/launcher_tests.rs (1)
57-80: LGTM!Also applies to: 83-107, 110-134, 137-155, 158-180, 183-201, 204-224, 821-849, 871-887, 890-949
crates/cli/src/config.rs (1)
426-428: LGTM!Also applies to: 443-451, 497-510, 627-633, 686-697, 713-721, 752-782, 790-821, 823-828, 861-875, 933-940, 976-980, 1047-1071
crates/cli/src/launcher.rs (1)
69-82: LGTM!docs/build-plugins/plugin-configuration-files.mdx (1)
130-139: LGTM!Also applies to: 346-347
Signed-off-by: Will Killian <wkillian@nvidia.com>
mnajafian-nv
left a comment
mnajafian-nv
left a comment
There was a problem hiding this comment.
LGTM overall. Thanks for driving this cleanup. Conditional approval pending review of the inline suggestions.
willkill07
changed the title
Signed-off-by: Will Killian <wkillian@nvidia.com>
lvojtku
left a comment
lvojtku
left a comment
There was a problem hiding this comment.
Approved with no comments
Signed-off-by: Will Killian <wkillian@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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/config.rs (1)
822-828: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the remaining request-header plugin override.
load_shared_confignow makesplugins.tomlthe plugin config source, butGatewayConfig::session_config_from_headersstill letsx-nemo-relay-plugin-configoverrideself.plugin_config. Any caller that can send gateway headers can still bypass the new source-of-truth. Drop that header branch or gate it behind a trusted/internal path.Proposed fix
- let plugin_config = header_json(headers, "x-nemo-relay-plugin-config") - .or_else(|| self.plugin_config.clone()); + let plugin_config = self.plugin_config.clone();🤖 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/config.rs` around lines 822 - 828, `GatewayConfig::session_config_from_headers` still allows `x-nemo-relay-plugin-config` to override `self.plugin_config`, which bypasses the new `plugins.toml` source of truth from `load_shared_config`. Remove that header-based override path, or restrict it to a trusted/internal-only flow, and keep `ResolvedConfig`/`apply_plugin_toml_config` as the only way plugin config is set.
🤖 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/launcher.rs`:
- Around line 718-721: The ATIF storage destination helper currently returns the
raw HTTP endpoint from AtifStorageConfig::Http, which can leak credentials or
tokens when printed. Update atif_storage_destination to emit a redacted display
form for the Http branch, preserving only safe parts of the URL, and keep the
existing S3 handling unchanged. Use the existing AtifStorageConfig and
HttpUploadConfig::resolve flow to locate the affected formatting path.
In `@crates/cli/tests/coverage/config_tests.rs`:
- Around line 326-368: The permission-based test in
unreadable_config_errors_include_the_source_path can fail unpredictably when run
as root because chmod 0o000 does not block reads for root. Add a runtime guard
in this test to skip or short-circuit when the effective user is root, or assert
the precondition before calling resolve_run_config and
load_plugin_toml_config_from_paths. Keep the existing assertions around the
config and plugin error paths, but ensure the unreadable-file setup is only
exercised when permissions will actually cause read failures.
---
Outside diff comments:
In `@crates/cli/src/config.rs`:
- Around line 822-828: `GatewayConfig::session_config_from_headers` still allows
`x-nemo-relay-plugin-config` to override `self.plugin_config`, which bypasses
the new `plugins.toml` source of truth from `load_shared_config`. Remove that
header-based override path, or restrict it to a trusted/internal-only flow, and
keep `ResolvedConfig`/`apply_plugin_toml_config` as the only way plugin config
is set.
🪄 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: 0d0c625a-85c7-47cf-9c55-2db458f3f012
📒 Files selected for processing (6)
crates/cli/src/config.rscrates/cli/src/installer.rscrates/cli/src/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_tests.rs
💤 Files with no reviewable changes (1)
- crates/cli/src/installer.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 (8)
**/*.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 work
**/*.rs: Usecargo fmt(rustfmt defaults) for Rust code changed in the core runtime or Rust-facing API surface.
Runcargo clippy -- -D warningsand keep Rust code warning-free; all warnings are treated as errors.
Use Rustsnake_casenaming conventions.Use
snake_casefor Rust identifiers.
Files:
crates/cli/src/launcher.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/src/config.rs
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Keep SPDX headers on source, docs, scripts, and configuration files.
Files:
crates/cli/src/launcher.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/src/config.rs
**/*.{rs,py,go,ts,js,mjs,cjs,jsx,tsx,c,h}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions: Rust and Python use
snake_case, C FFI exports use thenemo_relay_prefix, Go public APIs usePascalCase, and Node.js usescamelCase.
Files:
crates/cli/src/launcher.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/src/config.rs
crates/**/*.{rs,py,ts,js,c,h}
📄 CodeRabbit inference engine (AGENTS.md)
crates/**/*.{rs,py,ts,js,c,h}: 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 wrapper layers.
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/launcher.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/src/config.rs
**/*.{rs,go,js,ts,py,html,md,mdx,toml,yml,yaml,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Include SPDX license headers in all source files using the correct comment syntax for the file type.
Files:
crates/cli/src/launcher.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/src/config.rs
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
crates/**/src/**/*.rs: Keep the stable plugin boundary explicit: native plugins cross a C ABI, and worker plugins crossgrpc-v1. Do not pass Rust runtime types, trait objects, futures, or allocator-owned strings across the native dynamic-library boundary.
Manifest validation must cover kind, compatibility, load contract, integrity, capability mismatch, and disabled-plugin behavior.
The native loader must keep libraries alive until registered callbacks are cleared and must deregister plugin kinds before unload.
Runtime helpers must cover marks, scopes, continuations, and isolated scope stacks.
plugins list,plugins inspect, andplugins validatemust report lifecycle and compatibility status without leaking secret config.
The top-leveldoctorcommand must report resolved dynamic plugin and host configuration status.
Files:
crates/cli/src/launcher.rscrates/cli/src/config.rs
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
This file provides guidance to agents, including Claude Code and OpenAI Codex, when working in this repository.
Project Overview
NeMo Relay is a multi-language agent runtime framework for execution scopes, lifecycle events, middleware, plugins, and observability around tool and LLM calls. The core runtime is Rust. Primary supported bindings are Rust, Python, and Node.js. Go and the raw C FFI are experimental and source-first.
The shared runtime model is:
- Scope stacks decide where work belongs and which scope-local behavior is visible.
- Middleware registries decide what guardrails and intercepts run around managed calls.
- Plugins install reusable runtime behavior from configuration.
- Events record runtime behavior in ATOF form.
- Subscribers and exporters consume events in-process or export them to ATIF, OpenTelemetry, OpenInference, or other backends.
Repository Structure
The repository layout separates the Rust runtime, language bindings,
documentation, integrations, and agent-facing skills.crates/ core/ # Rust core runtime crate, published as nemo-relay adaptive/ # Adaptive runtime primitives and plugin components python/ # PyO3 native extension for the Python package ffi/ # Raw C ABI layer used by downstream bindings such as Go node/ # NAPI Node.js binding and JavaScript/TypeScript entry points python/ nemo_relay/ # Python wrapper package: scopes, tools, LLM, middleware, typed helpers, plugins, adaptive helpers tests/ # Python tests go/ nemo_relay/ # Experimental Go CGo binding and tests fern/ # Fern documentation site scripts/ # Stable wrappers and helper scripts; build/test/docs entry points live in justfile skills/ # Published Codex/agent skills for NeMo Relay usage patternsPrerequisites
Insta...
Files:
crates/cli/src/launcher.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/src/config.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/launcher_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rs
🔇 Additional comments (9)
crates/cli/tests/cli_tests.rs (2)
1487-1511: Good error-path coverage.New test validates the missing-explicit-config error path (message + path echoed in stderr), consistent with path instructions on covering error paths for changed API surface.
45-106: 📐 Maintainability & Code QualityLines 96-97 (
runtime = "command",entrypoint = "plugin.py") are marked as changed but aren't referenced by the line-range change details for this cohort (plugin_config_path migration). No functional issue found in the shown diff, but confirm this isn't an unrelated/incidental edit bleeding in from another commit in the stack.crates/cli/tests/coverage/config_tests.rs (2)
294-324: LGTM!
1338-1361: LGTM! Overrides correctly switched toplugin_config_path, andplugin_configassertions match the new resolution contract incrates/cli/src/config.rs.Also applies to: 1714-1735
crates/cli/tests/coverage/launcher_tests.rs (2)
397-440: Coverage aligned with ATIF exporter behavior.New
exporter_destinations_describe_atif_remote_storage_instead_of_local_pathtest correctly asserts remote storage lines and the absence of local filename-template reporting when storage is configured.
61-215: LGTM!plugin_config→plugin_config_pathrenames are mechanical and consistent with the updatedRunCommandstruct.Also applies to: 878-986
crates/cli/src/config.rs (2)
1039-1042: Already covered: explicit plugin config paths are still skipped when missing.This still calls
read_config_file(..., false, ...)for plugin config candidates, so the previous missing-override concern remains covered by the earlier review thread.
199-200: LGTM!Also applies to: 428-428, 443-451, 510-510, 629-633, 790-820, 833-855, 957-964, 1003-1003, 1029-1038, 1043-1083
crates/cli/src/launcher.rs (1)
8-10: LGTM!Also applies to: 563-563, 602-684, 715-717, 722-730, 783-798
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
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/config.rs (1)
822-828: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the remaining request-header plugin override.
load_shared_confignow makesplugins.tomlthe plugin config source, butGatewayConfig::session_config_from_headersstill letsx-nemo-relay-plugin-configoverrideself.plugin_config. Any caller that can send gateway headers can still bypass the new source-of-truth. Drop that header branch or gate it behind a trusted/internal path.Proposed fix
- let plugin_config = header_json(headers, "x-nemo-relay-plugin-config") - .or_else(|| self.plugin_config.clone()); + let plugin_config = self.plugin_config.clone();🤖 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/config.rs` around lines 822 - 828, `GatewayConfig::session_config_from_headers` still allows `x-nemo-relay-plugin-config` to override `self.plugin_config`, which bypasses the new `plugins.toml` source of truth from `load_shared_config`. Remove that header-based override path, or restrict it to a trusted/internal-only flow, and keep `ResolvedConfig`/`apply_plugin_toml_config` as the only way plugin config is set.
🤖 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/launcher.rs`:
- Around line 718-721: The ATIF storage destination helper currently returns the
raw HTTP endpoint from AtifStorageConfig::Http, which can leak credentials or
tokens when printed. Update atif_storage_destination to emit a redacted display
form for the Http branch, preserving only safe parts of the URL, and keep the
existing S3 handling unchanged. Use the existing AtifStorageConfig and
HttpUploadConfig::resolve flow to locate the affected formatting path.
In `@crates/cli/tests/coverage/config_tests.rs`:
- Around line 326-368: The permission-based test in
unreadable_config_errors_include_the_source_path can fail unpredictably when run
as root because chmod 0o000 does not block reads for root. Add a runtime guard
in this test to skip or short-circuit when the effective user is root, or assert
the precondition before calling resolve_run_config and
load_plugin_toml_config_from_paths. Keep the existing assertions around the
config and plugin error paths, but ensure the unreadable-file setup is only
exercised when permissions will actually cause read failures.
---
Outside diff comments:
In `@crates/cli/src/config.rs`:
- Around line 822-828: `GatewayConfig::session_config_from_headers` still allows
`x-nemo-relay-plugin-config` to override `self.plugin_config`, which bypasses
the new `plugins.toml` source of truth from `load_shared_config`. Remove that
header-based override path, or restrict it to a trusted/internal-only flow, and
keep `ResolvedConfig`/`apply_plugin_toml_config` as the only way plugin config
is set.
🪄 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: 0d0c625a-85c7-47cf-9c55-2db458f3f012
📒 Files selected for processing (6)
crates/cli/src/config.rscrates/cli/src/installer.rscrates/cli/src/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/config_tests.rscrates/cli/tests/coverage/launcher_tests.rs
💤 Files with no reviewable changes (1)
- crates/cli/src/installer.rs
📜 Review details
🔇 Additional comments (9)
crates/cli/tests/cli_tests.rs (2)
1487-1511: Good error-path coverage.New test validates the missing-explicit-config error path (message + path echoed in stderr), consistent with path instructions on covering error paths for changed API surface.
45-106: 📐 Maintainability & Code QualityLines 96-97 (
runtime = "command",entrypoint = "plugin.py") are marked as changed but aren't referenced by the line-range change details for this cohort (plugin_config_path migration). No functional issue found in the shown diff, but confirm this isn't an unrelated/incidental edit bleeding in from another commit in the stack.crates/cli/tests/coverage/config_tests.rs (2)
294-324: LGTM!
1338-1361: LGTM! Overrides correctly switched toplugin_config_path, andplugin_configassertions match the new resolution contract incrates/cli/src/config.rs.Also applies to: 1714-1735
crates/cli/tests/coverage/launcher_tests.rs (2)
397-440: Coverage aligned with ATIF exporter behavior.New
exporter_destinations_describe_atif_remote_storage_instead_of_local_pathtest correctly asserts remote storage lines and the absence of local filename-template reporting when storage is configured.
61-215: LGTM!plugin_config→plugin_config_pathrenames are mechanical and consistent with the updatedRunCommandstruct.Also applies to: 878-986
crates/cli/src/config.rs (2)
1039-1042: Already covered: explicit plugin config paths are still skipped when missing.This still calls
read_config_file(..., false, ...)for plugin config candidates, so the previous missing-override concern remains covered by the earlier review thread.
199-200: LGTM!Also applies to: 428-428, 443-451, 510-510, 629-633, 790-820, 833-855, 957-964, 1003-1003, 1029-1038, 1043-1083
crates/cli/src/launcher.rs (1)
8-10: LGTM!Also applies to: 563-563, 602-684, 715-717, 722-730, 783-798
🛑 Comments failed to post (2)
crates/cli/src/launcher.rs (1)
718-721: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the HTTP storage schema and any existing destination-rendering tests. rg -n -C4 'struct HttpStorageConfig|HttpStorageConfig|endpoint' crates/core/src/observability/plugin_component.rs rg -n -C4 'atif_storage_destination|exporter_destinations|render_status_frame' crates/cli/src/launcher.rs crates/cli/testsRepository: NVIDIA/NeMo-Relay
Length of output: 29687
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the status-banner tests and any existing URL redaction helpers. sed -n '703,740p' crates/cli/src/launcher.rs rg -n -C3 'redact|userinfo|password|token|credential|auth' crates/cli/src crates/core/src crates/cli/tests sed -n '398,440p' crates/cli/tests/coverage/launcher_tests.rsRepository: NVIDIA/NeMo-Relay
Length of output: 50376
🏁 Script executed:
#!/bin/bash set -euo pipefail # Probe URL parsing behavior relevant to ATIF HTTP storage endpoints. python3 - <<'PY' from urllib.parse import urlparse samples = [ "http://host/path", "https://user:pass@host/path", "https://host/path?token=secret", "https://user@host/path", ] for s in samples: u = urlparse(s) print(s) print(" scheme =", u.scheme) print(" netloc =", u.netloc) print(" username =", u.username) print(" password =", u.password) print(" geturl =", u.geturl()) PYRepository: NVIDIA/NeMo-Relay
Length of output: 691
Redact ATIF HTTP endpoints before printing
crates/cli/src/launcher.rs:718-721AtifStorageConfig::Httpaccepts full URLs, andHttpUploadConfig::resolvepreserves them, so printinghttp.endpointverbatim can echo userinfo or query tokens. Emit a redacted display form instead.🤖 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/launcher.rs` around lines 718 - 721, The ATIF storage destination helper currently returns the raw HTTP endpoint from AtifStorageConfig::Http, which can leak credentials or tokens when printed. Update atif_storage_destination to emit a redacted display form for the Http branch, preserving only safe parts of the URL, and keep the existing S3 handling unchanged. Use the existing AtifStorageConfig and HttpUploadConfig::resolve flow to locate the affected formatting path.crates/cli/tests/coverage/config_tests.rs (1)
326-368: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Permission-based tests may pass unintentionally when run as root.
unreadable_config_errors_include_the_source_pathrelies onchmod 0o000to force a read failure. Ifjust test-rust/ CI runs inside a container as root (common for Docker-based CI), root bypasses file permission bits entirely, so the file remains readable and bothunwrap_err()calls will panic instead of exercising the intended error path. Consider guarding with a root check (e.g., skip or usenix::unistd::Uid::effective().is_root()) or asserting a runtime precondition.🔧 Suggested guard
#[cfg(unix)] #[test] fn unreadable_config_errors_include_the_source_path() { use std::os::unix::fs::PermissionsExt; + if unsafe { libc::geteuid() } == 0 { + eprintln!("skipping: running as root, permission bits are not enforced"); + return; + } + let temp = tempfile::tempdir().unwrap();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.#[cfg(unix)] #[test] fn unreadable_config_errors_include_the_source_path() { use std::os::unix::fs::PermissionsExt; if unsafe { libc::geteuid() } == 0 { eprintln!("skipping: running as root, permission bits are not enforced"); return; } let temp = tempfile::tempdir().unwrap(); let config_path = temp.path().join("config.toml"); std::fs::write(&config_path, "").unwrap(); std::fs::set_permissions(&config_path, std::fs::Permissions::from_mode(0o000)).unwrap(); let command = RunCommand { agent: None, config: Some(config_path.clone()), openai_base_url: None, anthropic_base_url: None, session_metadata: None, plugin_config_path: None, dry_run: true, print: false, command: vec![], }; let config_error = resolve_run_config(&command, None).unwrap_err().to_string(); std::fs::set_permissions(&config_path, std::fs::Permissions::from_mode(0o600)).unwrap(); assert!(config_error.contains("failed to read configuration file")); assert!( config_error.contains(config_path.to_string_lossy().as_ref()), "{config_error}" ); let plugins_path = temp.path().join("plugins.toml"); std::fs::write(&plugins_path, "version = 1\n").unwrap(); std::fs::set_permissions(&plugins_path, std::fs::Permissions::from_mode(0o000)).unwrap(); let plugin_error = load_plugin_toml_config_from_paths(vec![plugins_path.clone()]) .unwrap_err() .to_string(); std::fs::set_permissions(&plugins_path, std::fs::Permissions::from_mode(0o600)).unwrap(); assert!(plugin_error.contains("failed to read plugin configuration file")); assert!( plugin_error.contains(plugins_path.to_string_lossy().as_ref()), "{plugin_error}" ); }🤖 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/config_tests.rs` around lines 326 - 368, The permission-based test in unreadable_config_errors_include_the_source_path can fail unpredictably when run as root because chmod 0o000 does not block reads for root. Add a runtime guard in this test to skip or short-circuit when the effective user is root, or assert the precondition before calling resolve_run_config and load_plugin_toml_config_from_paths. Keep the existing assertions around the config and plugin error paths, but ensure the unreadable-file setup is only exercised when permissions will actually cause read failures.
|
/merge |
5 participants
Overview
Streamline CLI plugin configuration around
plugins.tomland add a hidden path override for controlled runtime launches.Details
--plugin-config-pathsupport withNEMO_RELAY_PLUGIN_CONFIG_PATHfor daemon andrunconfiguration.--plugin-configCLI inputs, including hook forwarding.config.toml; plugin activation now comes fromplugins.toml.config.tomland replace--plugin-configusage with a plugin file.Validation:
just test-rustcargo clippy --workspace --all-targets -- -D warningsuv run pre-commit run --files ...just docsWhere should the reviewer start?
Start with
crates/cli/src/config.rsfor source precedence and removal of inline plugin configuration, then reviewcrates/cli/tests/coverage/config_tests.rsandcrates/cli/tests/cli_tests.rsfor the new path override and CLI behavior.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
plugins.toml, with support for an override that changes the load location.config.tomlno longer supports a[plugins]section; plugin configuration must live inplugins.toml.[plugins]section and focuses on updating the relevant[agents.*]blocks.--plugin-configand reflect the new configuration behavior.