feat(python): activate dynamic plugins#365
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPython now supports activating native and worker dynamic plugins through an owned host handle with discovered configuration layering, asynchronous close, context-manager support, exception mapping, typed declarations, and lifecycle tests. ChangesDynamic plugin host
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PythonCaller
participant PluginModule
participant NativeBinding
participant PluginHostActivation
PythonCaller->>PluginModule: activate_dynamic_plugins(config, dynamic_plugins)
PluginModule->>NativeBinding: normalize and activate specifications
NativeBinding->>PluginHostActivation: resolve discovered config and create owned activation
PluginHostActivation-->>PythonCaller: return activation report and handle
PythonCaller->>PluginHostActivation: close()
PluginHostActivation->>NativeBinding: clear activation on teardown thread
NativeBinding-->>PythonCaller: complete close
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:XL
a252f6a to
9615c1d
Compare
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
9615c1d to
8263c93
Compare
8263c93 to
d2a1746
Compare
d2a1746 to
776a701
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/core/src/plugin.rs`:
- Around line 986-993: Update lookup_plugin so it does not invoke
ensure_builtin_plugins_registered on every lookup. Ensure builtin registration
occurs through a one-time initialization path before lookups, while preserving
the existing read-lock lookup and Option<Arc<dyn Plugin>> behavior.
In `@python/nemo_relay/plugin.py`:
- Line 224: Update _normalize_object() so raw PluginConfig dictionaries still
omit entries whose values are None, while preserving None only for
component-local config maps or equivalent wrapper inputs consumed by
validate(...) and initialize(...). Ensure raw plugin configuration continues
deserializing as if null fields were omitted.
In `@python/tests/test_dynamic_plugin_host.py`:
- Around line 60-61: Rename the pytest fixtures native_dynamic_plugin and
worker_dynamic_plugin to use the repository convention: add the corresponding
name= argument to each `@pytest.fixture` decorator and rename the functions to
native_dynamic_plugin_fixture and worker_dynamic_plugin_fixture, preserving
their existing scopes, parameters, and behavior.
🪄 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: ee4c1bf3-cb01-420e-a23b-d63f9378a0f6
📒 Files selected for processing (24)
crates/cli/src/server.rscrates/cli/tests/coverage/server_tests.rscrates/core/Cargo.tomlcrates/core/src/observability/plugin_component.rscrates/core/src/plugin.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rscrates/core/src/plugins/model_pricing.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/tests/unit/plugin_tests.rscrates/python/src/py_plugin.rscrates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/_native.pyipython/nemo_relay/plugin.pypython/nemo_relay/plugin.pyipython/tests/test_dynamic_plugin_host.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (30)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/python/tests/coverage/py_plugin_coverage_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugin/dynamic/native.rscrates/python/src/py_plugin.rscrates/cli/src/server.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.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/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/Cargo.tomlcrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.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
Files:
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/python/tests/coverage/py_plugin_coverage_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugin/dynamic/native.rscrates/python/src/py_plugin.rscrates/cli/src/server.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rspython/tests/test_dynamic_plugin_host.pypython/nemo_relay/plugin.pycrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/python/tests/coverage/py_plugin_coverage_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugin/dynamic/native.rscrates/python/src/py_plugin.rscrates/cli/src/server.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rspython/tests/test_dynamic_plugin_host.pypython/nemo_relay/plugin.pycrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/python/tests/coverage/py_plugin_coverage_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugin/dynamic/native.rscrates/python/src/py_plugin.rscrates/cli/src/server.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rspython/tests/test_dynamic_plugin_host.pypython/nemo_relay/plugin.pycrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/python/tests/coverage/py_plugin_coverage_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugin/dynamic/native.rscrates/python/src/py_plugin.rscrates/cli/src/server.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/Cargo.tomlcrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/_native.pyicrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugin/dynamic/native.rscrates/python/src/py_plugin.rscrates/cli/src/server.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rspython/tests/test_dynamic_plugin_host.pypython/nemo_relay/plugin.pycrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.rspython/nemo_relay/plugin.pyi
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/Cargo.tomlcrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/python/tests/coverage/py_plugin_coverage_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugin/dynamic/native.rscrates/python/src/py_plugin.rscrates/cli/src/server.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rspython/tests/test_dynamic_plugin_host.pypython/nemo_relay/plugin.pycrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.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/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/Cargo.tomlcrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/_native.pyicrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugin/dynamic/native.rscrates/python/src/py_plugin.rscrates/cli/src/server.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rspython/tests/test_dynamic_plugin_host.pypython/nemo_relay/plugin.pycrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.rspython/nemo_relay/plugin.pyi
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/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rscrates/core/src/plugin.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/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/python/tests/coverage/py_plugin_coverage_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/cli/tests/coverage/server_tests.rscrates/core/tests/integration/worker_plugin_tests.rspython/tests/test_dynamic_plugin_host.pycrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/plugin_tests.rs
**/Cargo.toml
📄 CodeRabbit inference engine (.agents/skills/prepare-code-freeze/SKILL.md)
Confirm or infer the target release version from
upstream/main:Cargo.toml. Derive the release branch asrelease/<major>.<minor>.Keep Rust package names and workspace metadata in
Cargo.tomlinternally consistent across the project.
Files:
crates/core/Cargo.toml
**/*.toml
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all TOML files using the
#comment form.
Files:
crates/core/Cargo.toml
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/src/plugins/model_pricing.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/python/src/py_plugin.rscrates/cli/src/server.rscrates/core/src/plugin/dynamic/worker.rspython/tests/test_dynamic_plugin_host.pypython/nemo_relay/plugin.pycrates/core/src/plugin.rs
crates/{python,ffi,node}/**/*
⚙️ CodeRabbit configuration file
crates/{python,ffi,node}/**/*: Treat binding changes as public API changes. Check for parity with the other language bindings, FFI ownership/lifetime safety,
callback error propagation, stable type conversion, and consistent async/stream semantics.
Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere.
Files:
crates/python/tests/coverage/py_plugin_coverage_tests.rscrates/python/src/py_plugin.rs
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
python/nemo_relay/_native.pyipython/nemo_relay/plugin.pypython/nemo_relay/plugin.pyi
{python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update language wrapper helpers such as Python wrapper modules, Python type stubs, and Go shorthand packages when the new behavior belongs in those helper layers.
Files:
python/nemo_relay/_native.pyipython/nemo_relay/plugin.pypython/nemo_relay/plugin.pyi
python/nemo_relay/**/*
⚙️ CodeRabbit configuration file
python/nemo_relay/**/*: Review Python wrapper changes for typed API consistency, contextvars-based scope isolation, async behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/nemo_relay/_native.pyipython/nemo_relay/plugin.pypython/nemo_relay/plugin.pyi
{crates/core/src/plugin/dynamic/**,crates/plugin/**,crates/worker/**,crates/worker-proto/**,crates/types/**,python/plugin/**,examples/rust-native-plugin/**,examples/python-grpc-worker-plugin/**,docs/build-plugins/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Keep the stable boundary explicit: native plugins cross a C ABI, and worker plugins cross
grpc-v1.
Files:
crates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rs
{crates/core/src/plugin/dynamic/**/*.rs,examples/rust-native-plugin/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not pass Rust runtime types, trait objects, futures, or allocator-owned strings across the native dynamic-library boundary.
Files:
crates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rs
{crates/core/src/plugin/dynamic/**,examples/rust-native-plugin/**,examples/python-grpc-worker-plugin/**,docs/build-plugins/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Native and worker plugins are trusted extensions; document that native plugins are in-process and unsandboxed, and worker plugins provide process isolation but not a security sandbox.
Files:
crates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rs
{crates/core/src/plugin/dynamic/**/*.rs,crates/plugin/**/*.rs,crates/worker/**/*.rs,crates/worker-proto/**/*.rs,python/plugin/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Manifest validation must cover kind, compatibility, load contract, integrity, capability mismatch, and disabled-plugin behavior.
Files:
crates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rs
crates/core/src/plugin/dynamic/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
The native loader must keep libraries alive until registered callbacks are cleared and must deregister plugin kinds before unload.
Files:
crates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rs
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: When changing the Python wrapper package, tests, or docs tooling, lint with Ruff (E,F,W,I), format with Ruff formatter (120-character lines, double quotes), and passtytype checking.
Add the SPDX license header to all Python source files using the#comment form.
Files:
python/tests/test_dynamic_plugin_host.pypython/nemo_relay/plugin.py
**/*.{py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Keep Python, Go, and Node.js config objects and subscriber/exporter methods aligned so all bindings expose the same logical knobs and semantics.
Files:
python/tests/test_dynamic_plugin_host.pypython/nemo_relay/plugin.py
python/tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/test-python-binding/SKILL.md)
python/tests/**/*.py: Pytest is used to run tests.
Do not add@pytest.mark.asyncioto any test; async tests are automatically detected and run by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecconstructor argument when necessary.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures; if a fixture is needed in multiple test files, place it in aconftest.pyfile.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the fixture function asdef <fixture_name>_fixture() -> <return_type>:; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
Files:
python/tests/test_dynamic_plugin_host.py
python/nemo_relay/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python wrapper modules live under
python/nemo_relay/, and the native extension is built fromcrates/pythonwithmaturin.
Files:
python/nemo_relay/plugin.py
python/nemo_relay/{adaptive.py,plugin.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Keep Python adaptive/plugin wrappers in
python/nemo_relay/adaptive.pyandpython/nemo_relay/plugin.pysynchronized with the shared adaptive/plugin boundary and lifecycle.
Files:
python/nemo_relay/plugin.py
🪛 ast-grep (0.44.1)
python/tests/test_dynamic_plugin_host.py
[error] 64-76: Command coming from incoming request
Context: subprocess.run(
[
os.environ.get("CARGO", "cargo"),
"build",
"--quiet",
"--manifest-path",
str(root / "crates/core/tests/fixtures/native_plugin/Cargo.toml"),
"--target-dir",
str(target),
],
cwd=root,
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 117-130: Command coming from incoming request
Context: subprocess.run(
[
os.environ.get("CARGO", "cargo"),
"build",
"--quiet",
"--locked",
"--manifest-path",
str(root / "crates/core/tests/fixtures/worker_plugin/Cargo.toml"),
"--target-dir",
str(target),
],
cwd=root,
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 380-388: Command coming from incoming request
Context: subprocess.Popen(
[
"/bin/sh",
"-c",
'sleep 0.8; kill -CONT "$1"',
"resume-worker",
str(worker_pid),
]
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.21)
python/tests/test_dynamic_plugin_host.py
[error] 65-65: subprocess call: check for execution of untrusted input
(S603)
[error] 118-118: subprocess call: check for execution of untrusted input
(S603)
[warning] 238-238: Missing return type annotation for private function validate
Add return type annotation: None
(ANN202)
[warning] 241-241: Missing return type annotation for private function register
Add return type annotation: None
(ANN202)
[warning] 242-242: Missing return type annotation for private function block
Add return type annotation: None
(ANN202)
[warning] 262-263: Use asyncio.Event instead of awaiting asyncio.sleep in a while loop
(ASYNC110)
[warning] 304-304: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 374-374: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
[warning] 381-381: Async functions should not create subprocesses with blocking methods
(ASYNC220)
[error] 381-381: subprocess call: check for execution of untrusted input
(S603)
[warning] 396-399: Use contextlib.suppress(ProcessLookupError) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(ProcessLookupError): ...
(SIM105)
python/nemo_relay/plugin.py
[warning] 15-15: Import from collections.abc instead: AsyncIterator, Callable
Import from collections.abc
(UP035)
🔇 Additional comments (24)
crates/core/src/plugin.rs (1)
52-94: LGTM!Also applies to: 852-908, 927-951, 1187-1389, 1537-1712, 1738-1768, 1777-1884
crates/core/src/plugin/dynamic.rs (1)
21-58: LGTM!crates/core/src/plugin/dynamic/host.rs (1)
33-367: LGTM!crates/core/src/plugin/dynamic/native.rs (1)
54-160: LGTM!crates/core/src/plugin/dynamic/worker.rs (1)
11-76: LGTM!Also applies to: 123-207, 265-424, 603-603
crates/cli/tests/coverage/server_tests.rs (1)
1705-1735: LGTM!crates/core/tests/integration/native_plugin_tests.rs (1)
27-144: LGTM!Also applies to: 1110-1613, 1649-1657, 1776-1789
python/tests/test_dynamic_plugin_host.py (1)
163-421: LGTM!crates/core/src/observability/plugin_component.rs (1)
58-58: LGTM!Also applies to: 547-549
crates/core/src/plugins/model_pricing.rs (1)
17-25: LGTM!crates/cli/src/server.rs (1)
15-16: LGTM!Also applies to: 149-150, 185-187, 317-379
crates/python/src/py_plugin.rs (1)
34-37: LGTM!Also applies to: 702-919, 921-950, 995-1006, 1023-1035
python/nemo_relay/plugin.py (1)
314-446: LGTM!python/nemo_relay/plugin.pyi (1)
5-6: LGTM!Also applies to: 25-25, 121-157
python/nemo_relay/_native.pyi (1)
2252-2284: LGTM!crates/python/tests/coverage/py_plugin_coverage_tests.rs (1)
78-91: LGTM!Also applies to: 112-128
crates/core/src/plugins/nemo_guardrails/component.rs (1)
18-18: LGTM!Also applies to: 401-401
crates/core/Cargo.toml (1)
124-127: 📐 Maintainability & Code QualityConfirm the required full validation matrix.
The supplied validation summary names Rust and Python checks only. Please confirm
validate-change,just test-go, andjust test-nodealso pass before merge.As per coding guidelines, changes to
crates/coremust run the full Rust, Python, Go, and Node.js validation matrix. As per path instructions, changes touchingcrates/coremust also usevalidate-change.Sources: Coding guidelines, Path instructions
crates/core/tests/fixtures/native_plugin/src/lib.rs (1)
283-299: LGTM!crates/core/tests/fixtures/worker_plugin/src/main.rs (1)
84-88: LGTM!Also applies to: 130-134, 154-160
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rs (1)
1-80: LGTM!crates/core/tests/integration/worker_plugin_tests.rs (1)
26-31: LGTM!Also applies to: 48-129
crates/core/tests/unit/dynamic_worker_tests.rs (1)
1478-1481: LGTM!crates/core/tests/unit/plugin_tests.rs (1)
12-12: LGTM!Also applies to: 30-40, 304-413, 433-436, 950-959, 975-978, 1027-1066, 1096-1152, 1154-1185, 1186-1207, 1208-1234, 1236-1271, 1272-1310
mnajafian-nv
left a comment
mnajafian-nv
left a comment
There was a problem hiding this comment.
Thanks, this looks close. I left one Python-surface comment on the public activation-handle contract.
zhongxuanwang-nv
left a comment
zhongxuanwang-nv
left a comment
There was a problem hiding this comment.
All nitty things. Used powerful AIs to help review. Thanks Bryan!
776a701 to
287b543
Compare
github-actions
Bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
python/nemo_relay/plugin.py (1)
224-224:⚠️ Potential issue | 🟠 MajorDict normalization no longer drops
None— same concern raised on a previous commit.
_normalize()'s dict branch now preservesNonefor every key, including when_normalize_object()is applied to rawPluginConfig-shaped dicts passed tovalidate()/initialize(). A prior review already traced this: Rust structs that only default missing top-level fields may fail to deserialize an explicitnullwhere omission previously worked. This is unresolved in the current diff — the same dict comprehension is unchanged from what was flagged.🤖 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 `@python/nemo_relay/plugin.py` at line 224, The dict branch of _normalize must omit entries whose normalized value is None, preserving omission semantics for PluginConfig-shaped dictionaries passed through validate() and initialize(). Update the comprehension or surrounding logic so non-None values remain normalized and retained while None-valued keys are dropped.
🤖 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/python/src/py_plugin.rs`:
- Around line 846-859: Add an operational alert in the spawn-failure branch of
the plugin teardown flow before forgetting the activation, using the existing
logging or metrics mechanism to record the leaked activation and failed dynamic
teardown task. Keep the current non-blocking cleanup behavior and
PluginTeardownError handling unchanged.
- Around line 713-761: Consolidate PluginError-to-Python-exception
classification by keeping PluginTeardownErrorKind,
PluginTeardownError::from_plugin_error, and PluginTeardownError::to_py_err as
the single source of truth at crates/python/src/py_plugin.rs lines 713-761; no
direct change is needed there. Replace the duplicated match in
plugin_error_to_py_err at crates/python/src/py_plugin.rs lines 1026-1038 with
delegation through PluginTeardownError::from_plugin_error(error).to_py_err(),
preserving the NotFound-to-PyFileNotFoundError mapping.
---
Duplicate comments:
In `@python/nemo_relay/plugin.py`:
- Line 224: The dict branch of _normalize must omit entries whose normalized
value is None, preserving omission semantics for PluginConfig-shaped
dictionaries passed through validate() and initialize(). Update the
comprehension or surrounding logic so non-None values remain normalized and
retained while None-valued keys are dropped.
🪄 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: 1b38e7fb-e5c9-469d-8afe-c5db7ffb0464
📒 Files selected for processing (6)
crates/python/src/py_plugin.rscrates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/_native.pyipython/nemo_relay/plugin.pypython/nemo_relay/plugin.pyipython/tests/test_dynamic_plugin_host.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (18)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/python/tests/coverage/py_plugin_coverage_tests.rscrates/python/src/py_plugin.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.pycrates/python/src/py_plugin.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.pycrates/python/src/py_plugin.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.pycrates/python/src/py_plugin.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/python/tests/coverage/py_plugin_coverage_tests.rscrates/python/src/py_plugin.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/plugin.pyipython/nemo_relay/_native.pyipython/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.pycrates/python/src/py_plugin.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.pycrates/python/src/py_plugin.rs
crates/{python,ffi,node}/**/*
⚙️ CodeRabbit configuration file
crates/{python,ffi,node}/**/*: Treat binding changes as public API changes. Check for parity with the other language bindings, FFI ownership/lifetime safety,
callback error propagation, stable type conversion, and consistent async/stream semantics.
Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere.
Files:
crates/python/tests/coverage/py_plugin_coverage_tests.rscrates/python/src/py_plugin.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/python/tests/coverage/py_plugin_coverage_tests.rspython/tests/test_dynamic_plugin_host.py
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
python/nemo_relay/plugin.pyipython/nemo_relay/_native.pyipython/nemo_relay/plugin.py
{python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update language wrapper helpers such as Python wrapper modules, Python type stubs, and Go shorthand packages when the new behavior belongs in those helper layers.
Files:
python/nemo_relay/plugin.pyipython/nemo_relay/_native.pyipython/nemo_relay/plugin.py
python/nemo_relay/**/*
⚙️ CodeRabbit configuration file
python/nemo_relay/**/*: Review Python wrapper changes for typed API consistency, contextvars-based scope isolation, async behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/nemo_relay/plugin.pyipython/nemo_relay/_native.pyipython/nemo_relay/plugin.py
python/nemo_relay/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python wrapper modules live under
python/nemo_relay/, and the native extension is built fromcrates/pythonwithmaturin.
Files:
python/nemo_relay/plugin.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: When changing the Python wrapper package, tests, or docs tooling, lint with Ruff (E,F,W,I), format with Ruff formatter (120-character lines, double quotes), and passtytype checking.
Add the SPDX license header to all Python source files using the#comment form.
Files:
python/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.py
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
python/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.pycrates/python/src/py_plugin.rs
**/*.{py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Keep Python, Go, and Node.js config objects and subscriber/exporter methods aligned so all bindings expose the same logical knobs and semantics.
Files:
python/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.py
python/nemo_relay/{adaptive.py,plugin.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Keep Python adaptive/plugin wrappers in
python/nemo_relay/adaptive.pyandpython/nemo_relay/plugin.pysynchronized with the shared adaptive/plugin boundary and lifecycle.
Files:
python/nemo_relay/plugin.py
python/tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/test-python-binding/SKILL.md)
python/tests/**/*.py: Pytest is used to run tests.
Do not add@pytest.mark.asyncioto any test; async tests are automatically detected and run by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecconstructor argument when necessary.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures; if a fixture is needed in multiple test files, place it in aconftest.pyfile.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the fixture function asdef <fixture_name>_fixture() -> <return_type>:; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
Files:
python/tests/test_dynamic_plugin_host.py
🪛 ast-grep (0.44.1)
python/tests/test_dynamic_plugin_host.py
[error] 64-76: Command coming from incoming request
Context: subprocess.run(
[
os.environ.get("CARGO", "cargo"),
"build",
"--quiet",
"--manifest-path",
str(root / "crates/core/tests/fixtures/native_plugin/Cargo.toml"),
"--target-dir",
str(target),
],
cwd=root,
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 117-130: Command coming from incoming request
Context: subprocess.run(
[
os.environ.get("CARGO", "cargo"),
"build",
"--quiet",
"--locked",
"--manifest-path",
str(root / "crates/core/tests/fixtures/worker_plugin/Cargo.toml"),
"--target-dir",
str(target),
],
cwd=root,
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 380-388: Command coming from incoming request
Context: subprocess.Popen(
[
"/bin/sh",
"-c",
'sleep 0.8; kill -CONT "$1"',
"resume-worker",
str(worker_pid),
]
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.21)
python/nemo_relay/plugin.py
[warning] 15-15: Import from collections.abc instead: AsyncIterator, Callable
Import from collections.abc
(UP035)
python/tests/test_dynamic_plugin_host.py
[error] 65-65: subprocess call: check for execution of untrusted input
(S603)
[error] 118-118: subprocess call: check for execution of untrusted input
(S603)
[warning] 238-238: Missing return type annotation for private function validate
Add return type annotation: None
(ANN202)
[warning] 241-241: Missing return type annotation for private function register
Add return type annotation: None
(ANN202)
[warning] 242-242: Missing return type annotation for private function block
Add return type annotation: None
(ANN202)
[warning] 262-263: Use asyncio.Event instead of awaiting asyncio.sleep in a while loop
(ASYNC110)
[warning] 304-304: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 374-374: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
[warning] 381-381: Async functions should not create subprocesses with blocking methods
(ASYNC220)
[error] 381-381: subprocess call: check for execution of untrusted input
(S603)
[warning] 396-399: Use contextlib.suppress(ProcessLookupError) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(ProcessLookupError): ...
(SIM105)
🔇 Additional comments (19)
python/nemo_relay/plugin.py (4)
425-450: Open design question from a prior review remains unaddressed: does this bypassplugins.toml-sourced base config?A previous reviewer asked whether
activate_dynamic_pluginsmay skip the config normally served fromplugins.tomlas the base config. Nothing in this diff visibly resolves that question —confighere is whatever the caller passes, with no merge against on-diskplugins.tomlstate.
314-391: LGTM!DynamicPluginActivationSpec/PluginHostActivationmatch the native stubs and test fixtures, and theis_activedocstring now correctly scopes the guarantee to "has not begun teardown" rather than implying process-wide availability.
552-558: LGTM!__all__matches the new public surface.
15-15: 📐 Maintainability & Code QualityNo issue Ruff only selects E/F/W/I here, and the repo requires Python >=3.11, so
typing.Selfis supported.> Likely an incorrect or invalid review comment.python/nemo_relay/plugin.pyi (1)
5-6: LGTM! Stub signatures forDynamicPluginKind,DynamicPluginActivationSpec,PluginHostActivation, andactivate_dynamic_pluginsmatch the runtime implementation inplugin.py. As per path instructions, "Stubs and runtime implementations should stay aligned," which holds here.Also applies to: 25-25, 121-157
python/nemo_relay/_native.pyi (1)
2252-2288: LGTM!_PluginHostActivationandactivate_dynamic_pluginsstubs match the PyO3 bindings incrates/python/src/py_plugin.rs(getter names,Awaitablereturn shapes, and documented exceptional flow).crates/python/src/py_plugin.rs (3)
762-923: LGTM! TheActive/Closing/Closedstate machine,tokio::watch-based completion signaling, andcatch_unwind-guarded background teardown correctly implement idempotent, cancellation-resistant, non-blocking close semantics — consistent withtest_concurrent_close_waiters_share_cancellation_resistant_teardownandtest_worker_activation_finalizer_never_waits_on_python_thread.
998-1010: LGTM!register()correctly exports_PluginHostActivationandactivate_dynamic_plugins_py.
924-954: 🗄️ Data Integrity & IntegrationNo action needed PyO3 0.29.0 already accepts
#[pyo3(signature = (...))]type hints withexperimental-inspect, andactivate_dynamic_pluginsis already surfaced through the Python binding/stub set.> Likely an incorrect or invalid review comment.crates/python/tests/coverage/py_plugin_coverage_tests.rs (1)
78-91: LGTM! Binding-registration list andplugin_error_to_py_errbranch coverage are consistent withcrates/python/src/py_plugin.rs'sregister()and error mapping.Also applies to: 112-128
python/tests/test_dynamic_plugin_host.py (9)
60-61: Fixture naming still doesn't follow the repo'sname=/_fixtureconvention.Same issue flagged in a prior review; still present. Applies to both
native_dynamic_plugin(here) andworker_dynamic_plugin(Lines 113-114). As per path instructions: "use@pytest.fixture(name=\"<fixture_name>\"[, scope=\"<scope>\"])and define the fixture function asdef <fixture_name>_fixture() -> <return_type>:".♻️ Naming fix
-@pytest.fixture(scope="session") -def native_dynamic_plugin(tmp_path_factory: pytest.TempPathFactory) -> _BuiltPlugin: +@pytest.fixture(name="native_dynamic_plugin", scope="session") +def native_dynamic_plugin_fixture(tmp_path_factory: pytest.TempPathFactory) -> _BuiltPlugin:Also applies to: 65-77
Source: Path instructions
113-114: Same fixture-naming issue asnative_dynamic_plugin(Lines 60-61).♻️ Naming fix
-@pytest.fixture(scope="session") -def worker_dynamic_plugin(tmp_path_factory: pytest.TempPathFactory) -> _BuiltPlugin: +@pytest.fixture(name="worker_dynamic_plugin", scope="session") +def worker_dynamic_plugin_fixture(tmp_path_factory: pytest.TempPathFactory) -> _BuiltPlugin:Source: Path instructions
1-57: LGTM!
163-227: LGTM!
230-281: LGTM!
284-328: LGTM!
331-347: LGTM!
410-421: LGTM!
360-368: 🎯 Functional CorrectnessNo change needed here The fixture writes the same
entrypoint = ...line this replacement targets, so thereplacecall cannot silently miss in this test.> Likely an incorrect or invalid review comment.
287b543 to
59f4b3b
Compare
github-actions
Bot
added
size:XL
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/core/tests/integration/native_plugin_tests.rs`:
- Around line 52-60: Refactor PluginDiscoveryTestEnv::enter and its callers to
avoid mutating process-global current_dir and XDG_CONFIG_HOME; pass the
discovery paths explicitly or execute the relevant test operation in a child
Command configured with current_dir and env. Remove the global
set_current_dir/set_var behavior while preserving the existing test setup and
cleanup semantics.
In `@python/tests/test_dynamic_plugin_host.py`:
- Around line 81-109: Use TOML-safe POSIX path strings when rendering both the
library and entrypoint values in the manifest fixtures: update the
manifest-generation code around the visible library path and the sibling
entrypoint site in python/tests/test_dynamic_plugin_host.py (lines 81-109 and
134-159) to serialize each path via its as_posix() form before applying
repr-style quoting, preserving the parsed paths on Windows.
🪄 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: d20ea169-db79-40af-a081-f263e2d7a0e7
📒 Files selected for processing (9)
crates/core/src/plugin.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.rscrates/python/src/py_plugin.rscrates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/_native.pyipython/nemo_relay/plugin.pypython/nemo_relay/plugin.pyipython/tests/test_dynamic_plugin_host.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (27)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/core/src/plugin.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.rscrates/python/src/py_plugin.rscrates/python/tests/coverage/py_plugin_coverage_tests.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.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.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
Files:
crates/core/src/plugin.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/src/plugin.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.rscrates/python/src/py_plugin.rscrates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.py
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/core/src/plugin.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.rscrates/python/src/py_plugin.rscrates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.py
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/core/src/plugin.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.rscrates/python/src/py_plugin.rscrates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.py
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/core/src/plugin.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.rscrates/python/src/py_plugin.rscrates/python/tests/coverage/py_plugin_coverage_tests.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/core/src/plugin.rscrates/core/src/plugin/dynamic/host.rscrates/python/src/py_plugin.rspython/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.py
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/core/src/plugin.rspython/nemo_relay/_native.pyicrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.rscrates/python/src/py_plugin.rscrates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/plugin.pyipython/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.py
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/core/src/plugin.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/core/src/plugin.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.rscrates/python/src/py_plugin.rscrates/python/tests/coverage/py_plugin_coverage_tests.rspython/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.py
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.rscrates/core/src/plugin/dynamic/host.rscrates/core/tests/integration/native_plugin_tests.rs
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
python/nemo_relay/_native.pyipython/nemo_relay/plugin.pyipython/nemo_relay/plugin.py
{python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update language wrapper helpers such as Python wrapper modules, Python type stubs, and Go shorthand packages when the new behavior belongs in those helper layers.
Files:
python/nemo_relay/_native.pyipython/nemo_relay/plugin.pyipython/nemo_relay/plugin.py
python/nemo_relay/**/*
⚙️ CodeRabbit configuration file
python/nemo_relay/**/*: Review Python wrapper changes for typed API consistency, contextvars-based scope isolation, async behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/nemo_relay/_native.pyipython/nemo_relay/plugin.pyipython/nemo_relay/plugin.py
{crates/core/src/plugin/dynamic/**,crates/plugin/**,crates/worker/**,crates/worker-proto/**,crates/types/**,python/plugin/**,examples/rust-native-plugin/**,examples/python-grpc-worker-plugin/**,docs/build-plugins/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Keep the stable boundary explicit: native plugins cross a C ABI, and worker plugins cross
grpc-v1.
Files:
crates/core/src/plugin/dynamic/host.rs
{crates/core/src/plugin/dynamic/**/*.rs,examples/rust-native-plugin/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not pass Rust runtime types, trait objects, futures, or allocator-owned strings across the native dynamic-library boundary.
Files:
crates/core/src/plugin/dynamic/host.rs
{crates/core/src/plugin/dynamic/**,examples/rust-native-plugin/**,examples/python-grpc-worker-plugin/**,docs/build-plugins/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Native and worker plugins are trusted extensions; document that native plugins are in-process and unsandboxed, and worker plugins provide process isolation but not a security sandbox.
Files:
crates/core/src/plugin/dynamic/host.rs
{crates/core/src/plugin/dynamic/**/*.rs,crates/plugin/**/*.rs,crates/worker/**/*.rs,crates/worker-proto/**/*.rs,python/plugin/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Manifest validation must cover kind, compatibility, load contract, integrity, capability mismatch, and disabled-plugin behavior.
Files:
crates/core/src/plugin/dynamic/host.rs
crates/core/src/plugin/dynamic/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
The native loader must keep libraries alive until registered callbacks are cleared and must deregister plugin kinds before unload.
Files:
crates/core/src/plugin/dynamic/host.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/core/tests/integration/native_plugin_tests.rscrates/python/tests/coverage/py_plugin_coverage_tests.rspython/tests/test_dynamic_plugin_host.py
crates/{python,ffi,node}/**/*
⚙️ CodeRabbit configuration file
crates/{python,ffi,node}/**/*: Treat binding changes as public API changes. Check for parity with the other language bindings, FFI ownership/lifetime safety,
callback error propagation, stable type conversion, and consistent async/stream semantics.
Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere.
Files:
crates/python/src/py_plugin.rscrates/python/tests/coverage/py_plugin_coverage_tests.rs
python/nemo_relay/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python wrapper modules live under
python/nemo_relay/, and the native extension is built fromcrates/pythonwithmaturin.
Files:
python/nemo_relay/plugin.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: When changing the Python wrapper package, tests, or docs tooling, lint with Ruff (E,F,W,I), format with Ruff formatter (120-character lines, double quotes), and passtytype checking.
Add the SPDX license header to all Python source files using the#comment form.
Files:
python/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.py
**/*.{py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Keep Python, Go, and Node.js config objects and subscriber/exporter methods aligned so all bindings expose the same logical knobs and semantics.
Files:
python/nemo_relay/plugin.pypython/tests/test_dynamic_plugin_host.py
python/nemo_relay/{adaptive.py,plugin.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Keep Python adaptive/plugin wrappers in
python/nemo_relay/adaptive.pyandpython/nemo_relay/plugin.pysynchronized with the shared adaptive/plugin boundary and lifecycle.
Files:
python/nemo_relay/plugin.py
python/tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/test-python-binding/SKILL.md)
python/tests/**/*.py: Pytest is used to run tests.
Do not add@pytest.mark.asyncioto any test; async tests are automatically detected and run by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecconstructor argument when necessary.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures; if a fixture is needed in multiple test files, place it in aconftest.pyfile.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the fixture function asdef <fixture_name>_fixture() -> <return_type>:; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
Files:
python/tests/test_dynamic_plugin_host.py
🪛 ast-grep (0.44.1)
python/tests/test_dynamic_plugin_host.py
[error] 64-76: Command coming from incoming request
Context: subprocess.run(
[
os.environ.get("CARGO", "cargo"),
"build",
"--quiet",
"--manifest-path",
str(root / "crates/core/tests/fixtures/native_plugin/Cargo.toml"),
"--target-dir",
str(target),
],
cwd=root,
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 117-130: Command coming from incoming request
Context: subprocess.run(
[
os.environ.get("CARGO", "cargo"),
"build",
"--quiet",
"--locked",
"--manifest-path",
str(root / "crates/core/tests/fixtures/worker_plugin/Cargo.toml"),
"--target-dir",
str(target),
],
cwd=root,
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 430-438: Command coming from incoming request
Context: subprocess.Popen(
[
"/bin/sh",
"-c",
'sleep 0.8; kill -CONT "$1"',
"resume-worker",
str(worker_pid),
]
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.21)
python/nemo_relay/plugin.py
[warning] 15-15: Import from collections.abc instead: AsyncIterator, Callable
Import from collections.abc
(UP035)
python/tests/test_dynamic_plugin_host.py
[error] 65-65: subprocess call: check for execution of untrusted input
(S603)
[error] 118-118: subprocess call: check for execution of untrusted input
(S603)
[warning] 238-238: Missing return type annotation for private function validate
Add return type annotation: None
(ANN202)
[warning] 241-241: Missing return type annotation for private function register
Add return type annotation: None
(ANN202)
[warning] 245-245: Boolean positional value in function call
(FBT003)
[warning] 288-288: Missing return type annotation for private function validate
Add return type annotation: None
(ANN202)
[warning] 291-291: Missing return type annotation for private function register
Add return type annotation: None
(ANN202)
[warning] 292-292: Missing return type annotation for private function block
Add return type annotation: None
(ANN202)
[warning] 312-313: Use asyncio.Event instead of awaiting asyncio.sleep in a while loop
(ASYNC110)
[warning] 354-354: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 424-424: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
[warning] 431-431: Async functions should not create subprocesses with blocking methods
(ASYNC220)
[error] 431-431: subprocess call: check for execution of untrusted input
(S603)
[warning] 446-449: Use contextlib.suppress(ProcessLookupError) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(ProcessLookupError): ...
(SIM105)
🔇 Additional comments (24)
python/nemo_relay/plugin.py (2)
224-224: Keep top-levelPluginConfignormalization distinct from component JSON.This still preserves
Nonein raw configuration fields such as{"policy": None}. Native deserialization defaults missing fields but rejects null fields, so omission semantics regress. Preserve nulls only in component-local configuration values.
15-15: LGTM!Also applies to: 34-37, 61-62, 85-88, 314-391, 552-559
crates/core/src/plugin/dynamic/host.rs (1)
21-21: LGTM!Also applies to: 82-107
python/nemo_relay/plugin.pyi (1)
5-6: LGTM!Also applies to: 25-25, 121-157
python/nemo_relay/_native.pyi (1)
2252-2288: LGTM!crates/core/src/plugin.rs (1)
1462-1465: 🗄️ Data Integrity & IntegrationTyped defaults are intended to override
plugins.tomlhere.initialize_pluginsdocuments that precedence, so serializingPluginConfigbefore layering is expected behavior.> Likely an incorrect or invalid review comment.crates/python/src/py_plugin.rs (9)
846-859: Thread-spawn failure permanently leaks the activation.If
std::thread::Builder::spawnfails, this intentionallymem::forgets the activation to avoid running teardown on the caller thread, meaning any worker subprocess/manifest state from that activation is never cleaned up for the life of the process. This was already flagged in a prior review round as worth an operator-visible alert given it only triggers under severe resource exhaustion.
1027-1039: Two previously-flagged concerns remain unresolved here.
- This match duplicates
PluginTeardownError::from_plugin_error/to_py_err(lines 713-761) verbatim — a prior review already noted this duplication was the root cause of a fix being applied in only one location, and recommended replacing this body withPluginTeardownError::from_plugin_error(error).to_py_err().PluginError::NotFound(_) => PyFileNotFoundError(line 1033) also fires for "plugin component not registered" lookups (a pure registry miss, unrelated to a missing file on disk), which a prior reviewer questioned distinguishing.Both remain applicable to this identical code.
34-37: LGTM!
702-761: LGTM!Also applies to: 860-883
763-845: Solid idempotent close-state machine. Mutex-guarded status transition +Arc<Mutex<Option<...>>>wrapper correctly prevents double-teardown and keeps the activation alive across the thread-spawn boundary without holding the lock across.await.
885-923: LGTM!
924-954: 🗄️ Data Integrity & IntegrationConfirm Go/Node parity tracking for the new dynamic-activation surface.
This adds a net-new capability (
_PluginHostActivation,activate_dynamic_plugins) exposed only through the Python binding in this PR stack. As per path instructions, "Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere," and per coding guidelines, "Keep Python, Go, and Node.js config objects and subscriber/exporter methods aligned so all bindings expose the same logical knobs and semantics." Given the review-stack outline shows this cohort is Python-only by design, please confirm Go/Node dynamic-plugin-activation parity is tracked in a follow-up stack rather than silently dropped.Sources: Coding guidelines, Path instructions
924-954: 🩺 Stability & AvailabilityVerify cancellation safety during activation itself, not just teardown.
The teardown path (
close()) is explicitly designed and tested to be cancellation-resistant (sharedwatchchannel, background thread continues regardless of the awaiting task being cancelled). This function's own.awaitonPluginHostActivation::activate_with_discovered_config(...)at line 941-943 has no equivalent guard: if the Python-side awaitable is cancelled mid-activation (e.g. asyncio task cancellation before any_PluginHostActivationhandle exists), it's unclear whether partial registrations get rolled back or leak, since that logic lives incrates/core/src/plugin/dynamic/host.rs(not in this file). Please confirm the core activation future is itself cancellation-safe/rolls back on drop.
999-1004: LGTM!python/tests/test_dynamic_plugin_host.py (8)
60-61: Fixture naming doesn't follow repo convention.
native_dynamic_pluginshould use thename=/_fixtureform per a prior review round; still unaddressed.
113-114: Fixture naming doesn't follow repo convention.
worker_dynamic_pluginshould use thename=/_fixtureform per a prior review round; still unaddressed.
28-58: LGTM!
163-197: LGTM!
200-278: LGTM!
280-332: LGTM!
334-397: LGTM!
400-472: LGTM!crates/python/tests/coverage/py_plugin_coverage_tests.rs (1)
71-130: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/tests/test_dynamic_plugin_host.py (2)
386-389: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the wait for teardown to begin.
This polling loop can hang the entire test worker indefinitely if
close()never transitionsis_activetoFalse. Use a deadline orasyncio.wait_for(...)so lifecycle regressions fail diagnostically instead of hanging CI.🤖 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 `@python/tests/test_dynamic_plugin_host.py` around lines 386 - 389, Bound the polling loop after creating first_close in the activation teardown test so it cannot wait indefinitely for activation.is_active to become false. Use a timeout or deadline around the wait, and ensure timeout failure reports the lifecycle regression diagnostically while preserving cancellation and cleanup of first_close.
373-406: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuarantee deregistration when activation or teardown fails.
plugin.deregister(plugin_kind)is skipped if activation fails before thetryblock or ifactivation.close()raises. That leaves process-global plugin state registered and can contaminate later tests. Initializeactivationbefore thetry, then place deregistration in an outerfinallyaround close cleanup.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 `@python/tests/test_dynamic_plugin_host.py` around lines 373 - 406, Restructure the test cleanup so plugin.deregister(plugin_kind) always executes, including when activate_dynamic_plugins or activation.close() fails. Initialize activation before the existing try block, then wrap the activation close cleanup in an outer finally that deregisters the plugin, while preserving the current second_close cancellation and release handling.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@python/tests/test_dynamic_plugin_host.py`:
- Around line 386-389: Bound the polling loop after creating first_close in the
activation teardown test so it cannot wait indefinitely for activation.is_active
to become false. Use a timeout or deadline around the wait, and ensure timeout
failure reports the lifecycle regression diagnostically while preserving
cancellation and cleanup of first_close.
- Around line 373-406: Restructure the test cleanup so
plugin.deregister(plugin_kind) always executes, including when
activate_dynamic_plugins or activation.close() fails. Initialize activation
before the existing try block, then wrap the activation close cleanup in an
outer finally that deregisters the plugin, while preserving the current
second_close cancellation and release handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 34bea9aa-94f6-4e24-abdc-80d0731c480a
📒 Files selected for processing (2)
crates/python/src/py_plugin.rspython/tests/test_dynamic_plugin_host.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/python/src/py_plugin.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/python/src/py_plugin.rspython/tests/test_dynamic_plugin_host.py
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/python/src/py_plugin.rspython/tests/test_dynamic_plugin_host.py
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/python/src/py_plugin.rspython/tests/test_dynamic_plugin_host.py
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/python/src/py_plugin.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/python/src/py_plugin.rspython/tests/test_dynamic_plugin_host.py
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/python/src/py_plugin.rspython/tests/test_dynamic_plugin_host.py
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/python/src/py_plugin.rspython/tests/test_dynamic_plugin_host.py
crates/{python,ffi,node}/**/*
⚙️ CodeRabbit configuration file
crates/{python,ffi,node}/**/*: Treat binding changes as public API changes. Check for parity with the other language bindings, FFI ownership/lifetime safety,
callback error propagation, stable type conversion, and consistent async/stream semantics.
Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere.
Files:
crates/python/src/py_plugin.rs
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: When changing the Python wrapper package, tests, or docs tooling, lint with Ruff (E,F,W,I), format with Ruff formatter (120-character lines, double quotes), and passtytype checking.
Add the SPDX license header to all Python source files using the#comment form.
Files:
python/tests/test_dynamic_plugin_host.py
**/*.{py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Keep Python, Go, and Node.js config objects and subscriber/exporter methods aligned so all bindings expose the same logical knobs and semantics.
Files:
python/tests/test_dynamic_plugin_host.py
python/tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/test-python-binding/SKILL.md)
python/tests/**/*.py: Pytest is used to run tests.
Do not add@pytest.mark.asyncioto any test; async tests are automatically detected and run by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecconstructor argument when necessary.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures; if a fixture is needed in multiple test files, place it in aconftest.pyfile.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the fixture function asdef <fixture_name>_fixture() -> <return_type>:; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
Files:
python/tests/test_dynamic_plugin_host.py
{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:
python/tests/test_dynamic_plugin_host.py
🔇 Additional comments (9)
python/tests/test_dynamic_plugin_host.py (5)
61-61: Fixture naming convention remains unresolved.Use
@pytest.fixture(name="native_dynamic_plugin")andnative_dynamic_plugin_fixture, with the equivalentworker_dynamic_plugin_fixtureform. This repeats the existing review finding.As per coding guidelines: “When creating a fixture, use
@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the fixture function asdef <fixture_name>_fixture() -> <return_type>:.”Also applies to: 114-114
Source: Coding guidelines
105-105: LGTM!Also applies to: 156-156
163-198: LGTM!Also applies to: 200-273
275-354: LGTM!
409-546: LGTM!crates/python/src/py_plugin.rs (4)
34-37: LGTM!
785-845: LGTM!Also applies to: 860-915
924-955: LGTM!
1001-1004: LGTM!Also applies to: 1027-1029
|
/ok to test 86c3d41 |
## Overview Adds a shared harness-native activation path that layers explicit plugin configuration over the default discovered plugins.toml files before loading dynamic plugins. The existing exact activation path remains available for hosts such as the Relay CLI that already resolved their configuration. ## Details - Reuse the same one-time config resolution used by static initialization. - Add PluginHostActivation::activate_with_discovered_config for language and FFI bindings. - Keep PluginHostActivation::activate exact and free of implicit discovery. - Validate that a file-configured static component and a dynamic component activate in one owned transaction. ## Validation - cargo test -p nemo-relay --test native_plugin_integration (28 passed) - cargo clippy -p nemo-relay --all-targets -- -D warnings - cargo fmt --all -- --check - git diff --check ## Related Issues Relates to #365, #366, and #368. ## Summary by CodeRabbit * **New Features** * Added a plugin activation entrypoint that layers discovered `plugins.toml` configuration with dynamically provided plugin components. * **Refactor** * Improved configuration resolution by centralizing how default/discovered configuration is merged with provided settings before activation. * **Bug Fixes** * Ensured activation cleanup correctly deregisters the discovered static-base plugin kind after clearing, without impacting dynamic registrations. * **Tests** * Added an end-to-end integration test covering combined discovered + dynamic activation and verifying post-clear deregistration. Authors: - Bryan Bednarski (https://github.com/bbednarski9) Approvers: - Will Killian (https://github.com/willkill07) URL: #418
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
86c3d41 to
ae60c7c
Compare
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
|
/merge |
4 participants
Overview
This adds an owned Python binding for initializing Relay configuration with explicit native and worker dynamic plugins. It builds on the shared process-wide host lifecycle merged in #364 and the discovered-configuration layering merged in #418.
initialize_with_dynamic_plugins(...)is the owned initialization path when at least one dynamic specification is present. It resolves discoveredplugins.tomlconfiguration once, layers the explicit base configuration over it, then initializes statically registered components before appending dynamic-plugin components in specification order.Static-only harness-native applications continue to use
plugin.initialize()andplugin.clear().Details
DynamicPluginActivationSpecand asyncinitialize_with_dynamic_plugins(...)tonemo_relay.plugin.PluginHostActivationwith the validation report and activation-handle state.close(),async with, and best-effort finalization without blocking the Python finalizer or holding the GIL during teardown.Nonefields are omitted, while component-localconfigmaps preserve nestedNoneas JSONnull.plugin.initialize()andplugin.clear()APIs. Empty dynamic initialization is rejected without claiming process ownership, so static initialization remains available.Where should the reviewer start?
Start with
python/nemo_relay/plugin.pyfor the publicinitialize_with_dynamic_plugins(...)API andcrates/python/src/py_plugin.rsfor ownership, cancellation-resistant cleanup, and exception translation. End-to-end fixture cases are inpython/tests/test_dynamic_plugin_host.py; core discovered-configuration layering is covered by #418.Validation
python/tests/test_dynamic_plugin_host.py— 14 passedcargo test -p nemo-relay-python— 63 unit tests and 1 integration test passedcargo clippy -p nemo-relay-python --all-targets -- -D warningscargo fmt --checkgit diff --checkRelated Issues
Builds on #364 and #418.