feat: add dynamic plugin lifecycle CLI commands#292
Conversation
Signed-off-by: Alex Fournier <[email protected]>
Signed-off-by: Alex Fournier <[email protected]>
WalkthroughIntroduces full dynamic plugin lifecycle CLI commands ( ChangesDynamic Plugin Lifecycle CLI
Sequence Diagram(s)sequenceDiagram
participant User
participant run_plugins
participant lifecycle as plugins::lifecycle::*
participant load_and_hydrate_scopes
participant ScopedRegistry
participant config_io as plugins::config_io
User->>run_plugins: plugins <subcommand> [--json]
run_plugins->>run_plugins: json_context() → Option~PluginJsonContext~
run_plugins->>lifecycle: add/validate/list/inspect/enable/disable/remove(command, server)
lifecycle->>load_and_hydrate_scopes: resolve_plugins_config + load_scoped_registries
load_and_hydrate_scopes-->>lifecycle: Vec~ScopedRegistry~
lifecycle->>ScopedRegistry: mutate records
lifecycle->>config_io: append/remove_dynamic_plugin_reference(plugins_toml)
lifecycle->>ScopedRegistry: save()
alt save fails
lifecycle->>config_io: restore_plugins_toml (rollback)
end
alt --json flag
lifecycle->>lifecycle: print_response_json(envelope)
lifecycle-->>run_plugins: Ok or CliError::PluginLifecycle
run_plugins->>lifecycle: render_plugin_error → ExitCode
else terminal output
lifecycle->>User: rendered text output
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:XL
Signed-off-by: Alex Fournier <[email protected]>
Signed-off-by: Alex Fournier <[email protected]>
Signed-off-by: Alex Fournier <[email protected]>
|
/ok to test 086832f |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/plugins/config_io.rs`:
- Around line 181-185: The issue is that DynamicPluginManifest::load_from_path
is being called without error handling, causing the entire removal operation to
abort if any manifest in the plugins.dynamic list is stale or broken, even if
that manifest is not the target plugin being removed. To fix this, modify the
code around the DynamicPluginManifest::load_from_path call to handle the error
gracefully instead of propagating it. When a manifest cannot be loaded, attempt
to match the target plugin using the registry's recorded source or manifest
reference instead of loading the full manifest. For non-target entries that
cannot be read, log a warning and continue scanning rather than failing the
whole remove operation. This allows the requested plugin to be removed even when
unrelated broken manifests exist in the configuration.
- Around line 165-170: The code currently returns Ok(false) when the [plugins]
section is not a table or when plugins.dynamic is not an array, which silently
masks configuration errors. Instead of treating these as missing sections,
return proper error results that indicate the configuration is malformed. In the
first else block (when root_table.get_mut("plugins") doesn't yield a Table), and
in the second else block (when plugins.get_mut("dynamic") doesn't yield an
Array), return an Err with a descriptive error message explaining that the
configuration format is invalid, so callers can properly handle and report the
configuration problem rather than proceeding as if the sections simply don't
exist.
In `@crates/cli/src/plugins/lifecycle.rs`:
- Around line 64-76: The rollback logic in the error handler after
scopes[scope_index].save() fails is using remove_dynamic_plugin_reference to
undo the append_dynamic_plugin_reference call, but this approach deletes all
references matching the plugin_id, including any pre-existing manual entries in
plugins.toml. Instead, capture a snapshot of the plugins_toml_path file before
calling append_dynamic_plugin_reference at line 74, then on save failure restore
the exact previous snapshot content rather than attempting selective removal.
This ensures only the newly appended reference is rolled back without affecting
any pre-existing configuration.
In `@crates/cli/src/plugins/lifecycle/render.rs`:
- Around line 123-132: The host_config_json formatting is currently exposing the
full plugin configuration including potential secrets by cloning and
pretty-printing the entire config object. Replace the map chain starting with
plugin.config.clone() through the serde_json::to_string_pretty call to instead
return a redacted placeholder string such as "<redacted>" rather than the actual
configuration content, ensuring sensitive plugin secrets are not exposed in
terminal output or support bundles while maintaining consistent JSON-style
formatting in the terminal inspect output.
In `@crates/cli/src/plugins/lifecycle/responses.rs`:
- Around line 208-210: The host_config field at line 209 is directly serializing
plugin.config without redacting sensitive information like credentials or tokens
that could be exposed in CI logs through the inspect --json output. Replace the
direct clone of plugin.config with a redaction function that masks or removes
sensitive values from the configuration, only exposing full values if an
explicit reveal flag is provided. Apply this same redaction logic consistently
to both the JSON serialization in the host_config mapping and to any terminal or
text-based inspect output to ensure credentials and tokens are never
inadvertently exposed.
In `@crates/cli/src/plugins/lifecycle/state.rs`:
- Around line 68-85: The save() method writes directly to self.state_path
without atomic operations, which can corrupt the registry if the process crashes
or disk fills during write. Instead of writing directly to self.state_path,
write the rendered content to a temporary file in the same directory (obtained
from self.state_path.parent()), ensure the file is flushed to disk, and then
atomically replace the original file using a rename operation. This ensures the
state file is only updated when the write is completely successful.
In `@crates/cli/src/plugins/lifecycle/target.rs`:
- Around line 22-29: The `looks_like_path` function in the target.rs file uses
`path.exists()` to determine if a string is a path, which creates ambiguity when
a plugin has a registered ID like "acme.plugin" and a file with that same name
exists in the current directory. Remove the `path.exists()` check and rely only
on explicit path syntax indicators such as the `.toml` file extension and path
separators (`/` or `\` or the platform-specific separator). This ensures that
registered plugin IDs are not incorrectly treated as paths merely because a
same-named file happens to exist in the filesystem.
In `@crates/cli/tests/coverage/main_tests.rs`:
- Around line 37-129: The dispatch tests for run_plugins are reading ambient
user/global plugin configuration from the environment, which causes flakiness
outside clean CI environments. Use the existing env lock strategy to set HOME
and XDG_CONFIG_HOME environment variables to temporary directories before
executing the test assertions. This will isolate the tests from ambient
configuration and ensure deterministic behavior for the exit-code assertions on
missing plugins in the dispatch test cases.
In `@crates/cli/tests/coverage/plugins_lifecycle_tests.rs`:
- Around line 68-73: The test suite is not fully isolating the environment for
plugin config resolution. While CurrentDirGuard handles changing the current
working directory, the HOME and XDG_CONFIG_HOME environment variables are not
being controlled, allowing ambient user/global plugin configs to leak into the
test and break assertions on plugin counts and lookups. Create an additional
environment guard similar to CurrentDirGuard that sets both HOME and
XDG_CONFIG_HOME to temporary directory paths for the duration of the test,
ensuring complete isolation of the plugin configuration environment. This guard
should be applied alongside or as part of the existing setup in this test suite
where resolve_plugins_config(None) is called.
🪄 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: 7d25b628-5095-4042-995a-b0ff0aa260ce
📒 Files selected for processing (15)
crates/cli/src/config.rscrates/cli/src/error.rscrates/cli/src/main.rscrates/cli/src/plugins.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/lifecycle.rscrates/cli/src/plugins/lifecycle/render.rscrates/cli/src/plugins/lifecycle/responses.rscrates/cli/src/plugins/lifecycle/state.rscrates/cli/src/plugins/lifecycle/target.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/main_tests.rscrates/cli/tests/coverage/plugins_lifecycle_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/core/src/plugin/dynamic/registry.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
**/*.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/core/src/plugin/dynamic/registry.rscrates/cli/tests/coverage/main_tests.rscrates/cli/src/main.rscrates/cli/src/plugins/lifecycle/target.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/src/error.rscrates/cli/src/plugins/lifecycle/render.rscrates/cli/src/plugins.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/lifecycle/responses.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/state.rscrates/cli/tests/coverage/plugins_lifecycle_tests.rscrates/cli/src/plugins/lifecycle.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/core/src/plugin/dynamic/registry.rscrates/cli/tests/coverage/main_tests.rscrates/cli/src/main.rscrates/cli/src/plugins/lifecycle/target.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/src/error.rscrates/cli/src/plugins/lifecycle/render.rscrates/cli/src/plugins.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/lifecycle/responses.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/state.rscrates/cli/tests/coverage/plugins_lifecycle_tests.rscrates/cli/src/plugins/lifecycle.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/core/src/plugin/dynamic/registry.rscrates/cli/tests/coverage/main_tests.rscrates/cli/src/main.rscrates/cli/src/plugins/lifecycle/target.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/src/error.rscrates/cli/src/plugins/lifecycle/render.rscrates/cli/src/plugins.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/lifecycle/responses.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/state.rscrates/cli/tests/coverage/plugins_lifecycle_tests.rscrates/cli/src/plugins/lifecycle.rscrates/cli/src/config.rs
{crates/core,crates/adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/src/plugin/dynamic/registry.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/core/src/plugin/dynamic/registry.rscrates/cli/tests/coverage/main_tests.rscrates/cli/src/main.rscrates/cli/src/plugins/lifecycle/target.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/src/error.rscrates/cli/src/plugins/lifecycle/render.rscrates/cli/src/plugins.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/lifecycle/responses.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/state.rscrates/cli/tests/coverage/plugins_lifecycle_tests.rscrates/cli/src/plugins/lifecycle.rscrates/cli/src/config.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation
crates/core/**/*.rs: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths. Keep errors explicit and binding-appropriate at the wrapper layer.
Files:
crates/core/src/plugin/dynamic/registry.rs
crates/{core,adaptive}/**
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full matrix across Rust, Python, Go, Node.js, and WebAssembly
Files:
crates/core/src/plugin/dynamic/registry.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/core/src/plugin/dynamic/registry.rscrates/cli/tests/coverage/main_tests.rscrates/cli/src/main.rscrates/cli/src/plugins/lifecycle/target.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/src/error.rscrates/cli/src/plugins/lifecycle/render.rscrates/cli/src/plugins.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/lifecycle/responses.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/state.rscrates/cli/tests/coverage/plugins_lifecycle_tests.rscrates/cli/src/plugins/lifecycle.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/core/src/plugin/dynamic/registry.rscrates/cli/tests/coverage/main_tests.rscrates/cli/src/main.rscrates/cli/src/plugins/lifecycle/target.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/src/error.rscrates/cli/src/plugins/lifecycle/render.rscrates/cli/src/plugins.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/lifecycle/responses.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/state.rscrates/cli/tests/coverage/plugins_lifecycle_tests.rscrates/cli/src/plugins/lifecycle.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/core/src/plugin/dynamic/registry.rscrates/cli/tests/coverage/main_tests.rscrates/cli/src/main.rscrates/cli/src/plugins/lifecycle/target.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/src/error.rscrates/cli/src/plugins/lifecycle/render.rscrates/cli/src/plugins.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/lifecycle/responses.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/state.rscrates/cli/tests/coverage/plugins_lifecycle_tests.rscrates/cli/src/plugins/lifecycle.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, WebAssembly, 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,
integration patches, 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 wasm/ # wasm-bindgen WebAssembly binding and JS wrappers 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 third_party/ # P...
Files:
crates/core/src/plugin/dynamic/registry.rscrates/cli/tests/coverage/main_tests.rscrates/cli/src/main.rscrates/cli/src/plugins/lifecycle/target.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/src/error.rscrates/cli/src/plugins/lifecycle/render.rscrates/cli/src/plugins.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/lifecycle/responses.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/state.rscrates/cli/tests/coverage/plugins_lifecycle_tests.rscrates/cli/src/plugins/lifecycle.rscrates/cli/src/config.rs
crates/{core,adaptive}/**/*.rs
⚙️ CodeRabbit configuration file
crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/core/src/plugin/dynamic/registry.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/main_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/plugins_lifecycle_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/main_tests.rscrates/cli/tests/coverage/plugins_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/plugins_lifecycle_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/src/plugins/config_io.rscrates/cli/src/config.rs
🔇 Additional comments (14)
crates/cli/src/config.rs (1)
200-244: LGTM!Also applies to: 341-420, 734-740
crates/cli/src/error.rs (1)
8-18: LGTM!Also applies to: 39-45, 60-73
crates/core/src/plugin/dynamic/registry.rs (1)
25-41: 📐 Maintainability & Code QualityPlease confirm the required core-change validation matrix was run.
This PR changes
crates/core, so please provide command results forcargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust,validate-change, and the affected language test matrix (just test-python,just test-go,just test-node,just test-wasm) before merge.As per coding guidelines, "
Any Rust change must run just test-rust", "Run cargo fmt --all", "Run cargo clippy --workspace --all-targets -- -D warnings", and forcrates/corechanges "also use validate-change" plus "run the full matrix across Rust, Python, Go, Node.js, and WebAssembly".Source: Coding guidelines
crates/cli/src/plugins/config_io.rs (2)
9-246: 📐 Maintainability & Code QualityConfirm the required Rust validation passed.
This cohort changes Rust files; please confirm
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust, anduv run pre-commit run --all-fileswere run before merge. As per coding guidelines,**/*.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”, and “Validate Rust code withuv run pre-commit run --all-files”.Source: Coding guidelines
9-17: LGTM!Also applies to: 29-54, 107-151, 209-246
crates/cli/src/plugins.rs (1)
25-25: LGTM!Also applies to: 95-95
crates/cli/src/plugins/lifecycle/target.rs (1)
1-20: LGTM!crates/cli/src/plugins/lifecycle/responses.rs (1)
1-207: LGTM!Also applies to: 215-316
crates/cli/src/plugins/lifecycle/render.rs (1)
1-122: LGTM!Also applies to: 137-273
crates/cli/tests/cli_tests.rs (2)
36-67: LGTM!Also applies to: 133-301
133-145: 📐 Maintainability & Code QualityConfirm these Rust validation commands were executed for this change.
Required checks for any Rust changes in this repository:
just test-rustcargo fmt --allcargo clippy --workspace --all-targets -- -D warningsuv run pre-commit run --all-filescrates/cli/tests/coverage/plugins_tests.rs (1)
6-7: LGTM!Also applies to: 94-122
crates/cli/src/plugins/lifecycle.rs (1)
1-508: 📐 Maintainability & Code QualityConfirm the required Rust validation passed.
This cohort changes Rust CLI code; please record/pass
just test-rust,cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, anduv run pre-commit run --all-filesbefore merge.As per coding guidelines, "
**/*.rs: Any Rust change must runjust test-rust", "cargo fmt --all", "cargo clippy --workspace --all-targets -- -D warnings", and "Validate Rust code withuv run pre-commit run --all-files".Source: Coding guidelines
crates/cli/src/main.rs (1)
87-87: LGTM!Also applies to: 104-138
| let Some(toml::Value::Table(plugins)) = root_table.get_mut("plugins") else { | ||
| return Ok(false); | ||
| }; | ||
| let Some(toml::Value::Array(dynamic_entries)) = plugins.get_mut("dynamic") else { | ||
| return Ok(false); | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return config errors for malformed plugin containers.
Line 165 and Line 168 treat non-table [plugins] or non-array plugins.dynamic as “not found”. That can let remove callers proceed while a malformed host config remains unmodified.
Proposed fix
- let Some(toml::Value::Table(plugins)) = root_table.get_mut("plugins") else {
+ let Some(plugins_value) = root_table.get_mut("plugins") else {
return Ok(false);
};
- let Some(toml::Value::Array(dynamic_entries)) = plugins.get_mut("dynamic") else {
+ let plugins = plugins_value.as_table_mut().ok_or_else(|| {
+ CliError::Config(format!(
+ "invalid plugin TOML in {}: [plugins] must be a table",
+ path.display()
+ ))
+ })?;
+ let Some(dynamic_value) = plugins.get_mut("dynamic") else {
return Ok(false);
};
+ let dynamic_entries = dynamic_value.as_array_mut().ok_or_else(|| {
+ CliError::Config(format!(
+ "invalid plugin TOML in {}: plugins.dynamic must be an array of tables",
+ path.display()
+ ))
+ })?;🤖 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/plugins/config_io.rs` around lines 165 - 170, The code
currently returns Ok(false) when the [plugins] section is not a table or when
plugins.dynamic is not an array, which silently masks configuration errors.
Instead of treating these as missing sections, return proper error results that
indicate the configuration is malformed. In the first else block (when
root_table.get_mut("plugins") doesn't yield a Table), and in the second else
block (when plugins.get_mut("dynamic") doesn't yield an Array), return an Err
with a descriptive error message explaining that the configuration format is
invalid, so callers can properly handle and report the configuration problem
rather than proceeding as if the sections simply don't exist.
| let keep = match manifest_ref { | ||
| Some(manifest_ref) => { | ||
| let (manifest, _) = DynamicPluginManifest::load_from_path(&manifest_ref) | ||
| .map_err(|error| CliError::Config(error.to_string()))?; | ||
| manifest.plugin.id.trim() != plugin_id |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not require every manifest to load during removal.
Line 183 aborts on the first stale or moved manifest while scanning all plugins.dynamic entries. One broken, unrelated reference can block removing the requested plugin; a broken target reference also cannot be cleaned up. Match the target using the registry’s recorded source/manifest ref when available, and retain or warn on unreadable non-target entries instead of failing the whole remove.
🤖 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/plugins/config_io.rs` around lines 181 - 185, The issue is
that DynamicPluginManifest::load_from_path is being called without error
handling, causing the entire removal operation to abort if any manifest in the
plugins.dynamic list is stale or broken, even if that manifest is not the target
plugin being removed. To fix this, modify the code around the
DynamicPluginManifest::load_from_path call to handle the error gracefully
instead of propagating it. When a manifest cannot be loaded, attempt to match
the target plugin using the registry's recorded source or manifest reference
instead of loading the full manifest. For non-target entries that cannot be
read, log a warning and continue scanning rather than failing the whole remove
operation. This allows the requested plugin to be removed even when unrelated
broken manifests exist in the configuration.
| let (plugins_toml_path, state_path, scope) = | ||
| scoped_paths_for_add(target_scope(&command.scope)?, server.config.as_ref())?; | ||
| let scope_index = ensure_scope(&mut scopes, scope, plugins_toml_path.clone(), state_path); | ||
| let record = validated_record_from_manifest(manifest, manifest_ref.clone())?; | ||
|
|
||
| scopes[scope_index] | ||
| .registry | ||
| .add(record) | ||
| .map_err(|error| CliError::Config(error.to_string()))?; | ||
| append_dynamic_plugin_reference(&plugins_toml_path, &manifest_ref)?; | ||
| if let Err(error) = scopes[scope_index].save() { | ||
| let _ = remove_dynamic_plugin_reference(&plugins_toml_path, &plugin_id); | ||
| return Err(error); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restore the exact TOML snapshot on add rollback.
Line 75 rolls back a failed state save by removing every reference that resolves to plugin_id. If plugins.toml already had a manual or duplicate reference for that id, rollback deletes pre-existing config instead of only undoing this append. Snapshot and restore the file, as remove already does.
Proposed fix
let (plugins_toml_path, state_path, scope) =
scoped_paths_for_add(target_scope(&command.scope)?, server.config.as_ref())?;
let scope_index = ensure_scope(&mut scopes, scope, plugins_toml_path.clone(), state_path);
let record = validated_record_from_manifest(manifest, manifest_ref.clone())?;
+ let original_plugins_toml = match std::fs::read(&plugins_toml_path) {
+ Ok(bytes) => Some(bytes),
+ Err(error) if error.kind() == std::io::ErrorKind::NotFound => None,
+ Err(error) => return Err(error.into()),
+ };
scopes[scope_index]
.registry
.add(record)
.map_err(|error| CliError::Config(error.to_string()))?;
- append_dynamic_plugin_reference(&plugins_toml_path, &manifest_ref)?;
+ if let Err(error) = append_dynamic_plugin_reference(&plugins_toml_path, &manifest_ref) {
+ let _ = restore_plugins_toml(&plugins_toml_path, original_plugins_toml.as_deref());
+ return Err(error);
+ }
if let Err(error) = scopes[scope_index].save() {
- let _ = remove_dynamic_plugin_reference(&plugins_toml_path, &plugin_id);
+ let _ = restore_plugins_toml(&plugins_toml_path, original_plugins_toml.as_deref());
return Err(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/src/plugins/lifecycle.rs` around lines 64 - 76, The rollback logic
in the error handler after scopes[scope_index].save() fails is using
remove_dynamic_plugin_reference to undo the append_dynamic_plugin_reference
call, but this approach deletes all references matching the plugin_id, including
any pre-existing manual entries in plugins.toml. Instead, capture a snapshot of
the plugins_toml_path file before calling append_dynamic_plugin_reference at
line 74, then on save failure restore the exact previous snapshot content rather
than attempting selective removal. This ensures only the newly appended
reference is rolled back without affecting any pre-existing configuration.
| lines.push(format!( | ||
| "host_config_json: {}", | ||
| host_config | ||
| .map(|plugin| plugin.config.clone()) | ||
| .filter(|config| !config.is_empty()) | ||
| .map(|config| { | ||
| serde_json::to_string_pretty(&Value::Object(config)) | ||
| .expect("host config serializes") | ||
| }) | ||
| .unwrap_or_else(|| "<none>".into()) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact host config in terminal inspect output.
Line 126 clones and pretty-prints the full host config. Keep terminal output consistent with JSON by showing status or a redacted placeholder; otherwise plugin secrets can be copied into logs or support bundles.
🤖 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/plugins/lifecycle/render.rs` around lines 123 - 132, The
host_config_json formatting is currently exposing the full plugin configuration
including potential secrets by cloning and pretty-printing the entire config
object. Replace the map chain starting with plugin.config.clone() through the
serde_json::to_string_pretty call to instead return a redacted placeholder
string such as "<redacted>" rather than the actual configuration content,
ensuring sensitive plugin secrets are not exposed in terminal output or support
bundles while maintaining consistent JSON-style formatting in the terminal
inspect output.
| host_config: host_config | ||
| .map(|plugin| Value::Object(plugin.config.clone())) | ||
| .unwrap_or(Value::Null), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact host config in JSON inspect output.
Line 209 serializes plugin.config directly. Dynamic plugin config can contain credentials or tokens, and inspect --json output is easy to capture in CI logs. Emit status, keys, or redacted values unless an explicit reveal flag exists, and apply the same redaction policy to terminal inspect output.
🤖 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/plugins/lifecycle/responses.rs` around lines 208 - 210, The
host_config field at line 209 is directly serializing plugin.config without
redacting sensitive information like credentials or tokens that could be exposed
in CI logs through the inspect --json output. Replace the direct clone of
plugin.config with a redaction function that masks or removes sensitive values
from the configuration, only exposing full values if an explicit reveal flag is
provided. Apply this same redaction logic consistently to both the JSON
serialization in the host_config mapping and to any terminal or text-based
inspect output to ensure credentials and tokens are never inadvertently exposed.
| pub(super) fn save(&self) -> Result<(), CliError> { | ||
| let rendered = serde_json::to_vec_pretty(&PersistedDynamicPluginRegistry { | ||
| schema_version: DYNAMIC_PLUGIN_STATE_SCHEMA_VERSION, | ||
| records: self.registry.cloned_records(true), | ||
| }) | ||
| .map_err(|error| { | ||
| CliError::Config(format!( | ||
| "could not serialize dynamic plugin registry state {}: {error}", | ||
| self.state_path.display() | ||
| )) | ||
| })?; | ||
| if let Some(parent) = self.state_path.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| } | ||
| let mut rendered = rendered; | ||
| rendered.push(b'\n'); | ||
| std::fs::write(&self.state_path, rendered)?; | ||
| Ok(()) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use atomic replace for state-file writes to prevent registry corruption.
save() writes directly to dynamic-plugins.json. If the process crashes or the disk fills during write, the file can be left partially written, and read_registry() will fail on the next run. Write to a temp file in the same directory and atomically replace the destination only after a successful flush.
🤖 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/plugins/lifecycle/state.rs` around lines 68 - 85, The save()
method writes directly to self.state_path without atomic operations, which can
corrupt the registry if the process crashes or disk fills during write. Instead
of writing directly to self.state_path, write the rendered content to a
temporary file in the same directory (obtained from self.state_path.parent()),
ensure the file is flushed to disk, and then atomically replace the original
file using a rename operation. This ensures the state file is only updated when
the write is completely successful.
| fn looks_like_path(target: &str) -> bool { | ||
| let path = Path::new(target); | ||
| path.exists() | ||
| || target.ends_with(".toml") | ||
| || target.contains(std::path::MAIN_SEPARATOR) | ||
| || target.contains('/') | ||
| || target.contains('\\') | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid treating ambiguous IDs as paths just because a same-named file exists.
Line 24 makes PluginTarget::parse("acme.plugin") choose Path whenever the current directory contains an acme.plugin file or directory, so plugins validate acme.plugin skips the registered-ID branch and will not update registry validation state. Prefer explicit path syntax (./..., absolute paths, separators, .toml) or resolve registered IDs before falling back to an existing 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/plugins/lifecycle/target.rs` around lines 22 - 29, The
`looks_like_path` function in the target.rs file uses `path.exists()` to
determine if a string is a path, which creates ambiguity when a plugin has a
registered ID like "acme.plugin" and a file with that same name exists in the
current directory. Remove the `path.exists()` check and rely only on explicit
path syntax indicators such as the `.toml` file extension and path separators
(`/` or `\` or the platform-specific separator). This ensures that registered
plugin IDs are not incorrectly treated as paths merely because a same-named file
happens to exist in the filesystem.
| let error = run_plugins( | ||
| PluginsCommand { | ||
| command: PluginsSubcommand::Edit(PluginsEditCommand::default()), | ||
| }, | ||
| &ServerArgs::default(), | ||
| ) | ||
| .unwrap_err() | ||
| .to_string(); | ||
| assert!(error.contains("interactive terminal") || error.contains("TTY")); | ||
|
|
||
| assert_eq!( | ||
| run_plugins( | ||
| PluginsCommand { | ||
| command: PluginsSubcommand::List(PluginsListCommand::default()), | ||
| }, | ||
| &ServerArgs::default() | ||
| ) | ||
| .unwrap(), | ||
| ExitCode::SUCCESS | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| run_plugins( | ||
| PluginsCommand { | ||
| command: PluginsSubcommand::Inspect(PluginsInspectCommand { | ||
| id: "missing.plugin".into(), | ||
| json: false, | ||
| }), | ||
| }, | ||
| &ServerArgs::default(), | ||
| ) | ||
| .unwrap(), | ||
| ExitCode::from(2) | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| run_plugins( | ||
| PluginsCommand { | ||
| command: PluginsSubcommand::Validate(PluginsValidateCommand { | ||
| target: "missing.plugin".into(), | ||
| json: false, | ||
| }), | ||
| }, | ||
| &ServerArgs::default(), | ||
| ) | ||
| .unwrap(), | ||
| ExitCode::from(2) | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| run_plugins( | ||
| PluginsCommand { | ||
| command: PluginsSubcommand::List(PluginsListCommand { | ||
| all: false, | ||
| json: false, | ||
| }), | ||
| }, | ||
| &ServerArgs::default() | ||
| ) | ||
| .unwrap(), | ||
| ExitCode::SUCCESS | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn safe_dispatch_plugin_json_errors_return_exit_codes() { | ||
| assert_eq!( | ||
| run_plugins( | ||
| PluginsCommand { | ||
| command: PluginsSubcommand::Inspect(PluginsInspectCommand { | ||
| id: "missing.plugin".into(), | ||
| json: true, | ||
| }), | ||
| }, | ||
| &ServerArgs::default(), | ||
| ) | ||
| .unwrap(), | ||
| ExitCode::from(2) | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| run_plugins( | ||
| PluginsCommand { | ||
| command: PluginsSubcommand::Validate(PluginsValidateCommand { | ||
| target: "missing.plugin".into(), | ||
| json: true, | ||
| }), | ||
| }, | ||
| &ServerArgs::default(), | ||
| ) | ||
| .unwrap(), | ||
| ExitCode::from(2) | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make these dispatch tests environment-hermetic.
run_plugins(..., &ServerArgs::default()) can read ambient user/global plugin config, so exit-code assertions for missing plugins may become flaky outside clean CI environments. Set HOME/XDG_CONFIG_HOME to temp dirs (under the existing env lock strategy) for deterministic behavior.
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/main_tests.rs` around lines 37 - 129, The dispatch
tests for run_plugins are reading ambient user/global plugin configuration from
the environment, which causes flakiness outside clean CI environments. Use the
existing env lock strategy to set HOME and XDG_CONFIG_HOME environment variables
to temporary directories before executing the test assertions. This will isolate
the tests from ambient configuration and ensure deterministic behavior for the
exit-code assertions on missing plugins in the dispatch test cases.
Source: Path instructions
| let temp = tempfile::tempdir().unwrap(); | ||
| let _cwd = CurrentDirGuard::enter(temp.path()); | ||
| let plugin_dir = temp.path().join("plugins").join("acme"); | ||
| std::fs::create_dir_all(&plugin_dir).unwrap(); | ||
| write_dynamic_manifest(&plugin_dir, "acme.guardrail"); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Isolate lifecycle tests from ambient HOME/XDG config, not only CWD.
These tests frequently use resolve_plugins_config(None) after only changing current directory. Host user/global plugin configs can leak in and break assertions like exact plugin counts and specific-ID lookups. Add an env guard to set HOME and XDG_CONFIG_HOME to temp paths for this suite.
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/plugins_lifecycle_tests.rs` around lines 68 - 73,
The test suite is not fully isolating the environment for plugin config
resolution. While CurrentDirGuard handles changing the current working
directory, the HOME and XDG_CONFIG_HOME environment variables are not being
controlled, allowing ambient user/global plugin configs to leak into the test
and break assertions on plugin counts and lookups. Create an additional
environment guard similar to CurrentDirGuard that sets both HOME and
XDG_CONFIG_HOME to temporary directory paths for the duration of the test,
ensuring complete isolation of the plugin configuration environment. This guard
should be applied alongside or as part of the existing setup in this test suite
where resolve_plugins_config(None) is called.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/cli/src/plugins/lifecycle/target.rs (1)
38-42: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove
path.exists()check to prevent ID-to-path misclassification.Line 40 still uses
path.exists(), creating ambiguity when a registered plugin ID matches a filesystem entry name. If a user runsplugins validate acme.pluginand both a registered plugin "acme.plugin" and a file "acme.plugin" exist, the parser choosesPath, skipping the registry ID branch and preventing validation status persistence. The comment at lines 35-37 states the goal is to keep canonical IDs on the ID branch;path.exists()violates this. Remove the exists check and rely only on explicit path syntax (is_absolute()and the separator/prefix/suffix checks).🔧 Proposed fix
fn should_treat_target_as_path(target: &str) -> bool { let path = Path::new(target); - if path.exists() || path.is_absolute() { + if path.is_absolute() { return true; }🤖 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/plugins/lifecycle/target.rs` around lines 38 - 42, In the function should_treat_target_as_path, remove the path.exists() check from the condition on line 40. Keep only the path.is_absolute() check to determine if the target should be treated as a path. This prevents registered plugin IDs from being misclassified as filesystem paths when a file with the same name happens to exist, ensuring that canonical IDs are properly routed to the registry lookup branch instead of being incorrectly treated as filesystem paths.
🤖 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/plugins/lifecycle/target.rs`:
- Around line 116-124: The test
parse_treats_existing_filesystem_entries_as_paths is now invalid because it
relies on the removed path.exists() check that incorrectly treated "acme.worker"
as a path when it is actually a valid plugin ID format. Update the test by
passing explicit path syntax like "./acme.worker" to PluginTarget::parse()
instead of just the directory path string, so it correctly tests that the parser
treats explicitly-formatted paths as paths rather than IDs, or alternatively
remove the test entirely if it no longer serves a valid purpose after the parser
refactoring.
---
Duplicate comments:
In `@crates/cli/src/plugins/lifecycle/target.rs`:
- Around line 38-42: In the function should_treat_target_as_path, remove the
path.exists() check from the condition on line 40. Keep only the
path.is_absolute() check to determine if the target should be treated as a path.
This prevents registered plugin IDs from being misclassified as filesystem paths
when a file with the same name happens to exist, ensuring that canonical IDs are
properly routed to the registry lookup branch instead of being incorrectly
treated as filesystem paths.
🪄 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: 44dd9809-af44-4692-be9b-1cf25708201d
📒 Files selected for processing (1)
crates/cli/src/plugins/lifecycle/target.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (26)
- GitHub Check: Python / Test (macos-arm64)
- GitHub Check: Rust / Test (linux-amd64)
- GitHub Check: Node.js / Test (macos-arm64)
- GitHub Check: Rust / Test (linux-arm64)
- GitHub Check: Go / Test (windows-amd64)
- GitHub Check: Go / Test (macos-arm64)
- GitHub Check: Rust / Test (windows-arm64)
- GitHub Check: Go / Test (linux-amd64)
- GitHub Check: Rust / Test (windows-amd64)
- GitHub Check: Rust / Test (macos-arm64)
- GitHub Check: Go / Test (linux-arm64)
- GitHub Check: Go / Test (windows-arm64)
- GitHub Check: Node.js / Test (windows-amd64)
- GitHub Check: Node.js / Test (windows-arm64)
- GitHub Check: WebAssembly / Test (linux-amd64)
- GitHub Check: Node.js / Test (linux-arm64)
- GitHub Check: Node.js / Test (linux-amd64)
- GitHub Check: Python / Test (windows-arm64)
- GitHub Check: Python / Test (windows-amd64)
- GitHub Check: Python / Test (linux-amd64)
- GitHub Check: WebAssembly / Test (windows-amd64)
- GitHub Check: Python / Test (linux-arm64)
- GitHub Check: WebAssembly / Test (windows-arm64)
- GitHub Check: WebAssembly / Test (linux-arm64)
- GitHub Check: WebAssembly / Test (macos-arm64)
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (8)
**/*.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/plugins/lifecycle/target.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/plugins/lifecycle/target.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/plugins/lifecycle/target.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/plugins/lifecycle/target.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/plugins/lifecycle/target.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/plugins/lifecycle/target.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/plugins/lifecycle/target.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, WebAssembly, 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,
integration patches, 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 wasm/ # wasm-bindgen WebAssembly binding and JS wrappers 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 third_party/ # P...
Files:
crates/cli/src/plugins/lifecycle/target.rs
🔇 Additional comments (3)
crates/cli/src/plugins/lifecycle/target.rs (3)
12-19: LGTM!
21-33: LGTM!
12-52: 📐 Maintainability & Code QualityRun Rust validation for this change
Run
cargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warnings, andjust test-rustfor this Rust change.
| fn parse_treats_existing_filesystem_entries_as_paths() { | ||
| let temp = tempdir().unwrap(); | ||
| let existing = temp.path().join("acme.worker"); | ||
| std::fs::create_dir_all(&existing).unwrap(); | ||
| assert_eq!( | ||
| PluginTarget::parse(existing.to_str().unwrap()), | ||
| PluginTarget::Path(existing) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test validates incorrect exists() behavior.
This test expects an existing directory "acme.worker" to parse as Path, but "acme.worker" is a valid plugin ID format. After removing path.exists() from line 40, this test will fail because the parser will correctly treat "acme.worker" as an ID when no explicit path syntax is present. Either remove this test or update it to use explicit path syntax like "./acme.worker" if the goal is to test that existing paths with explicit syntax are classified as paths.
🔧 Option 1: Remove the test
- #[test]
- fn parse_treats_existing_filesystem_entries_as_paths() {
- let temp = tempdir().unwrap();
- let existing = temp.path().join("acme.worker");
- std::fs::create_dir_all(&existing).unwrap();
- assert_eq!(
- PluginTarget::parse(existing.to_str().unwrap()),
- PluginTarget::Path(existing)
- );
- }♻️ Option 2: Test explicit path syntax instead
#[test]
- fn parse_treats_existing_filesystem_entries_as_paths() {
+ fn parse_treats_explicit_relative_paths_with_existing_entries_as_paths() {
let temp = tempdir().unwrap();
let existing = temp.path().join("acme.worker");
std::fs::create_dir_all(&existing).unwrap();
+ let relative_path = format!("./{}", existing.file_name().unwrap().to_str().unwrap());
assert_eq!(
- PluginTarget::parse(existing.to_str().unwrap()),
- PluginTarget::Path(existing)
+ PluginTarget::parse(&relative_path),
+ PluginTarget::Path(PathBuf::from(relative_path))
);
}📝 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.
| fn parse_treats_existing_filesystem_entries_as_paths() { | |
| let temp = tempdir().unwrap(); | |
| let existing = temp.path().join("acme.worker"); | |
| std::fs::create_dir_all(&existing).unwrap(); | |
| assert_eq!( | |
| PluginTarget::parse(existing.to_str().unwrap()), | |
| PluginTarget::Path(existing) | |
| ); | |
| } | |
| #[test] | |
| fn parse_treats_explicit_relative_paths_with_existing_entries_as_paths() { | |
| let temp = tempdir().unwrap(); | |
| let existing = temp.path().join("acme.worker"); | |
| std::fs::create_dir_all(&existing).unwrap(); | |
| let relative_path = format!("./{}", existing.file_name().unwrap().to_str().unwrap()); | |
| assert_eq!( | |
| PluginTarget::parse(&relative_path), | |
| PluginTarget::Path(PathBuf::from(relative_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/plugins/lifecycle/target.rs` around lines 116 - 124, The test
parse_treats_existing_filesystem_entries_as_paths is now invalid because it
relies on the removed path.exists() check that incorrectly treated "acme.worker"
as a path when it is actually a valid plugin ID format. Update the test by
passing explicit path syntax like "./acme.worker" to PluginTarget::parse()
instead of just the directory path string, so it correctly tests that the parser
treats explicitly-formatted paths as paths rather than IDs, or alternatively
remove the test entirely if it no longer serves a valid purpose after the parser
refactoring.
2 participants
Overview
Add the dynamic plugin lifecycle CLI surface for RELAY-340.
Details
add,validate,list,inspect,enable,disable, andremovedynamic-plugins.jsonregistry file so desired-state mutations are durable across CLI runsplugins.tomldynamic plugin refs when no state file exists yetremovetombstone-based and add rollback for multi-file mutation failure on the host config pathplugins list --all, revival-aware add messaging, and richer inspect output for source and lifecycle stateplugins validate,plugins list, andplugins inspectplugins validate --jsonsuccess pathWhere should the reviewer start?
Start in
crates/cli/src/plugins/lifecycle.rs, then reviewcrates/cli/src/plugins/lifecycle/json.rsandcrates/cli/tests/coverage/plugins_lifecycle_tests.rs.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Release Notes
New Features
--json.--user/--project/--global.Bug Fixes