feat(python): activate dynamic plugins#365
Conversation
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 plugin.py
participant NativeBinding
participant PluginHostActivation
PythonCaller->>plugin.py: activate_dynamic_plugins(config, dynamic_plugins)
plugin.py->>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
🚥 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
bbednarski9
force-pushed
the
bbednarski/python-dynamic-plugin-host
branch
from
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 |
bbednarski9
force-pushed
the
bbednarski/python-dynamic-plugin-host
branch
from
9615c1d to
8263c93
Compare
bbednarski9
force-pushed
the
bbednarski/python-dynamic-plugin-host
branch
from
8263c93 to
d2a1746
Compare
bbednarski9
force-pushed
the
bbednarski/python-dynamic-plugin-host
branch
from
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
| return [_normalize(item) for item in value] | ||
| if isinstance(value, dict): | ||
| return {cast(str, key): _normalize(val) for key, val in value.items() if val is not None} | ||
| return {cast(str, key): _normalize(val) for key, val in value.items()} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find static callers relying on _normalize_object and any tests exercising None-valued static config.
rg -nP -C3 '_normalize_object\s*\(' python/nemo_relay
rg -nP -C3 'initialize\(|validate\(' python/tests | rg -n 'None'Repository: NVIDIA/NeMo-Relay
Length of output: 19704
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' python/nemo_relay/plugin.py
printf '\n---\n'
sed -n '260,480p' python/nemo_relay/plugin.py
printf '\n---\n'
sed -n '480,620p' python/nemo_relay/plugin.pyRepository: NVIDIA/NeMo-Relay
Length of output: 18304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Rust plugin config schema and null/option handling.
rg -n "struct .*Plugin|enum .*Plugin|Option<|serde_json::Value|JsonObject|initialize_plugins|validate_plugin_config|dynamic_plugins" crates python/nemo_relay -g '!target'Repository: NVIDIA/NeMo-Relay
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the plugin config types and inspect the relevant slices only.
ast-grep outline crates/types/src/plugin.rs --view expanded
printf '\n---\n'
sed -n '1,260p' crates/types/src/plugin.rs
printf '\n---\n'
rg -n -C3 'null|serde_json::Value::Null|from_value|validate_plugin_config|initialize_plugins' crates/**/tests crates/**/src -g '!target' | head -n 300Repository: NVIDIA/NeMo-Relay
Length of output: 22246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the Rust plugin configuration type and its serde shape.
rg -n "pub struct PluginConfig|pub struct PluginComponentSpec|pub struct DynamicPluginActivationSpec|pub struct PluginReport|type JsonObject" crates -g '!target' | head -n 80
printf '\n---\n'
sed -n '1,260p' crates/core/src/plugin.rsRepository: NVIDIA/NeMo-Relay
Length of output: 9348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect plugin tests that build static configs and any null-valued cases.
rg -n -C4 'PluginConfig\s*\{|PluginComponentSpec\s*\{|validate_plugin_config|initialize_plugins|Value::Null|None' \
crates/core/tests crates/adaptive/tests python/tests crates/python/src/py_plugin.rs \
-g '!target' | head -n 300Repository: NVIDIA/NeMo-Relay
Length of output: 20557
Keep None dropping for raw PluginConfig dicts _normalize_object() now preserves None everywhere it is used, but validate(...)/initialize(...) deserialize into Rust structs that only default missing top-level fields. A null in a raw plugin config can now fail where omission used to work; keep null-preservation limited to component-local config maps or handle wrapper inputs separately.
🤖 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, 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.
| @pytest.fixture(scope="session") | ||
| def native_dynamic_plugin(tmp_path_factory: pytest.TempPathFactory) -> _BuiltPlugin: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Fixture naming does not follow the repo convention.
native_dynamic_plugin and worker_dynamic_plugin should use the name=/_fixture form.
As per path instructions: "use @pytest.fixture(name="<fixture_name>"[, scope="<scope>"]) and define the fixture function as def <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:-@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:Also applies to: 113-114
🤖 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 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.
Source: Path instructions
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.
|
|
||
| @property | ||
| def is_active(self) -> bool: | ||
| """Return whether this object still owns the active plugin host.""" |
There was a problem hiding this comment.
This public contract reads a little stronger than the implementation. is_active flips to False as soon as close() starts, before the close awaitable has finished and before teardown is necessarily complete. The concurrent-close test already locks that behavior in. I’d tighten the docstring here so Python users read this as 'close has not started for this handle' rather than 'the process is ready for another activation.'
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!
| PluginError::InvalidConfig(_) | PluginError::Serialization(_) => { | ||
| pyo3::exceptions::PyValueError::new_err(message) | ||
| } | ||
| PluginError::NotFound(_) => pyo3::exceptions::PyFileNotFoundError::new_err(message), |
There was a problem hiding this comment.
PluginError::NotFound --> PyFileNotFoundError also fires for "plugin component 'x' is not registered" (plugin.rs:1793), e.g. a wrong static kind in the base config of activate_dynamic_plugins (validation only warns for unknown components). That reports a pure registry miss as a missing file. Should we consider distinguishing it?
| return cast(ConfigReport, await _initialize_plugins(_normalize_object(config))) | ||
|
|
||
|
|
||
| async def activate_dynamic_plugins( |
There was a problem hiding this comment.
activate_dynamic_plugins may skip the config from plugins.toml that was supposed to be served as the base config?
bbednarski9
force-pushed
the
bbednarski/python-dynamic-plugin-host
branch
from
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.
| #[derive(Clone, Copy)] | ||
| enum PluginTeardownErrorKind { | ||
| Value, | ||
| NotFound, | ||
| Runtime, | ||
| } | ||
|
|
||
| #[derive(Clone)] | ||
| struct PluginTeardownError { | ||
| kind: PluginTeardownErrorKind, | ||
| message: String, | ||
| } | ||
|
|
||
| impl PluginTeardownError { | ||
| fn from_plugin_error(error: PluginError) -> Self { | ||
| let message = error.to_string(); | ||
| let kind = match error { | ||
| PluginError::InvalidConfig(_) | PluginError::Serialization(_) => { | ||
| PluginTeardownErrorKind::Value | ||
| } | ||
| PluginError::NotFound(_) => PluginTeardownErrorKind::NotFound, | ||
| PluginError::Conflict(_) | ||
| | PluginError::Internal(_) | ||
| | PluginError::RegistrationFailed(_) => PluginTeardownErrorKind::Runtime, | ||
| }; | ||
| Self { kind, message } | ||
| } | ||
|
|
||
| fn runtime(message: impl Into<String>) -> Self { | ||
| Self { | ||
| kind: PluginTeardownErrorKind::Runtime, | ||
| message: message.into(), | ||
| } | ||
| } | ||
|
|
||
| fn to_py_err(&self) -> PyErr { | ||
| match self.kind { | ||
| PluginTeardownErrorKind::Value => { | ||
| pyo3::exceptions::PyValueError::new_err(self.message.clone()) | ||
| } | ||
| PluginTeardownErrorKind::NotFound => { | ||
| pyo3::exceptions::PyFileNotFoundError::new_err(self.message.clone()) | ||
| } | ||
| PluginTeardownErrorKind::Runtime => { | ||
| pyo3::exceptions::PyRuntimeError::new_err(self.message.clone()) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two independent implementations of the same PluginError → Python-exception classification. PluginTeardownError::from_plugin_error/to_py_err() and plugin_error_to_py_err both match the identical set of PluginError variants into the same three exception kinds; the duplication is the root cause of the NotFound→PyFileNotFoundError semantic issue only being reviewed and fixed in one location previously.
crates/python/src/py_plugin.rs#L713-L761: keep this as the single source of truth for the classification (PluginTeardownErrorKind/from_plugin_error/to_py_err).crates/python/src/py_plugin.rs#L1026-L1038: replace the duplicated match with a call into the logic above, e.g.PluginTeardownError::from_plugin_error(error).to_py_err().
📍 Affects 1 file
crates/python/src/py_plugin.rs#L713-L761(this comment)crates/python/src/py_plugin.rs#L1026-L1038
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/python/src/py_plugin.rs` around lines 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.
| if let Err(error) = spawn { | ||
| // Cleanup must never fall back to the Python thread. Retain the | ||
| // activation for process lifetime if no teardown thread can start. | ||
| if let Some(activation) = activation | ||
| .lock() | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()) | ||
| .take() | ||
| { | ||
| std::mem::forget(activation); | ||
| } | ||
| self.finish(Err(PluginTeardownError::runtime(format!( | ||
| "failed to start dynamic plugin teardown task: {error}" | ||
| )))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Thread-spawn failure permanently leaks the activation (documented, but worth alerting on).
If std::thread::Builder::spawn fails, the code intentionally mem::forgets the activation rather than run teardown on the caller thread — a deliberate tradeoff to avoid blocking, but it means any worker subprocess/manifest state from that activation is never cleaned up for the life of the process. Given this only triggers under severe resource exhaustion, consider surfacing a metric/log alert here so an operator can notice a leaked host rather than silently losing teardown forever.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/python/src/py_plugin.rs` around lines 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.
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>
bbednarski9
force-pushed
the
bbednarski/python-dynamic-plugin-host
branch
from
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!
| fn enter(cwd: &Path, xdg_config_home: &Path) -> Self { | ||
| let guard = Self { | ||
| previous_cwd: std::env::current_dir().expect("current directory should resolve"), | ||
| previous_xdg_config_home: std::env::var_os("XDG_CONFIG_HOME"), | ||
| }; | ||
| std::env::set_current_dir(cwd).expect("test current directory should be set"); | ||
| // SAFETY: native plugin integration tests serialize environment changes | ||
| // with NATIVE_PLUGIN_TEST_LOCK and this guard restores the prior value. | ||
| unsafe { std::env::set_var("XDG_CONFIG_HOME", xdg_config_home) }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 \
'NATIVE_PLUGIN_TEST_LOCK|set_var|remove_var|var_os|set_current_dir|current_dir' \
crates/core/tests/integration/native_plugin_tests.rs crates/core/src/plugin.rs
fd Cargo.toml . --exec sh -c \
'printf "\n--- %s ---\n" "$1"; rg -n "edition\s*=" "$1"' sh {}Repository: NVIDIA/NeMo-Relay
Length of output: 23960
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' crates/core/tests/integration/native_plugin_tests.rs
printf '\n--- env access in core tests ---\n'
rg -n 'XDG_CONFIG_HOME|set_current_dir|current_dir\(|set_var\(|remove_var\(|var_os\(' crates/core/tests crates/core/src
printf '\n--- tokio test usage in this file ---\n'
rg -n '#\[tokio::test\]|blocking_lock\(|lock\.await|NATIVE_PLUGIN_TEST_LOCK' crates/core/tests/integration/native_plugin_tests.rsRepository: NVIDIA/NeMo-Relay
Length of output: 13773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'PluginDiscoveryTestEnv::enter|NATIVE_PLUGIN_TEST_LOCK' crates/core/tests/integration/native_plugin_tests.rs
python3 - <<'PY'
from pathlib import Path
text = Path('crates/core/tests/integration/native_plugin_tests.rs').read_text()
for i, line in enumerate(text.splitlines(), 1):
if 'PluginDiscoveryTestEnv::enter' in line:
print(i, line.strip())
PYRepository: NVIDIA/NeMo-Relay
Length of output: 8308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1218,1255p' crates/core/tests/integration/native_plugin_tests.rs
printf '\n--- all discovery helper call sites ---\n'
rg -n 'PluginDiscoveryTestEnv::enter' crates/core/tests/integration/native_plugin_tests.rs
printf '\n--- surrounding function for the discovery helper call site ---\n'
sed -n '1234,1252p' crates/core/tests/integration/native_plugin_tests.rsRepository: NVIDIA/NeMo-Relay
Length of output: 3069
Avoid process-global env/cwd mutation in this helper PluginDiscoveryTestEnv changes current_dir and XDG_CONFIG_HOME for the whole test process. The local mutex only serializes callers in this file, so unrelated tests or library code can still observe the temporary state; use a child process with Command::env/current_dir, or pass the discovery path explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/core/tests/integration/native_plugin_tests.rs` around lines 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.
| manifest = manifest_dir / "relay-plugin.toml" | ||
| manifest.write_text( | ||
| textwrap.dedent( | ||
| f""" | ||
| manifest_version = 1 | ||
|
|
||
| [plugin] | ||
| id = "fixture_native" | ||
| kind = "rust_dynamic" | ||
|
|
||
| [compat] | ||
| relay = "={_relay_version()}" | ||
| native_api = "1" | ||
|
|
||
| [defaults] | ||
| enabled = false | ||
|
|
||
| [capabilities] | ||
| items = ["plugin_native"] | ||
|
|
||
| [integrity] | ||
| sha256 = "sha256:{digest}" | ||
|
|
||
| [load] | ||
| library = {str(library)!r} | ||
| symbol = "nemo_relay_fixture_native_plugin" | ||
| """ | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('python/tests/test_dynamic_plugin_host.py')
print(p.exists())
print(p.read_text())
PYRepository: NVIDIA/NeMo-Relay
Length of output: 15595
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('python/tests/test_dynamic_plugin_host.py')
print("exists:", p.exists())
print("lines:", sum(1 for _ in p.open()))
for start, end in [(1, 220)]:
print(f"\n--- {start}-{end} ---")
for i, line in enumerate(p.open(), start=1):
if start <= i <= end:
print(f"{i:4d}: {line.rstrip()}")
PYRepository: NVIDIA/NeMo-Relay
Length of output: 7934
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('python/tests/test_dynamic_plugin_host.py')
print("exists:", p.exists())
print("lines:", sum(1 for _ in p.open()))
for start, end in [(1, 220)]:
print(f"\n--- {start}-{end} ---")
with p.open() as f:
for i, line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:4d}: {line.rstrip()}")
PYRepository: NVIDIA/NeMo-Relay
Length of output: 7934
🏁 Script executed:
rg -n "library\s*=|entrypoint\s*=|relay-plugin.toml|worker_protocol|rust_dynamic|kind = \"worker\"" crates python -g '!**/target/**'Repository: NVIDIA/NeMo-Relay
Length of output: 32001
🏁 Script executed:
python3 - <<'PY'
import tomllib
samples = [
r"C:\temp\lib.dll",
r"\\server\share\lib.dll",
r"C:\Program Files\Relay\plugin.exe",
]
for s in samples:
toml_text = f"library = {s!r}\n"
parsed = tomllib.loads(toml_text)["library"]
print("source:", s)
print("toml :", toml_text.strip())
print("parsed:", parsed)
print("equal :", parsed == s)
print("---")
PYRepository: NVIDIA/NeMo-Relay
Length of output: 554
Use TOML-safe path strings here. repr() emits a Python string literal, so the Windows backslashes in library and entrypoint survive into the parsed TOML value and the path changes. Use a TOML-safe form for both, e.g. path.as_posix()!r.
📍 Affects 1 file
python/tests/test_dynamic_plugin_host.py#L81-L109(this comment)python/tests/test_dynamic_plugin_host.py#L134-L159
🤖 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 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.
4 participants
Overview
This adds an owned Python binding for explicit native and worker dynamic-plugin activation. It is stacked on #364; once that PR lands, this diff reduces to the Python binding and its tests.
Static-only harness-native applications keep using
plugin.initialize()/plugin.clear().activate_dynamic_plugins(...)requires at least one dynamic specification; its base config may also contain static components, which initialize before components appended by the dynamic plugins.Details
DynamicPluginActivationSpecand asyncactivate_dynamic_plugins(...)tonemo_relay.plugin.PluginHostActivationwith the successful validation report and active-state inspection.close(),async with, and best-effort finalization without blocking the Python finalizer or holding the GIL during teardown.Nonevalues as JSONnullin plugin configuration while still omitting optional top-level specification fields.plugin.initialize()andplugin.clear()APIs. Empty dynamic activation is rejected without claiming process ownership, so static initialization remains available.Where should the reviewer start?
Start with
python/nemo_relay/plugin.pyfor the public API andcrates/python/src/py_plugin.rsfor shared ownership, cancellation-resistant cleanup, and exception translation. End-to-end fixture cases are inpython/tests/test_dynamic_plugin_host.py.Validation
python/tests/test_dynamic_plugin_host.py— 10 passedcargo fmt --all -- --checkcargo clippy -p nemo-relay-python --all-targets -- -D warningsgit diff --checkRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to #364.
Summary by CodeRabbit
None.