feat(plugin): add owned dynamic host activation#364
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe plugin system now uses ownership-tracked registrations, serialized mutation transactions, dynamic host activation handles, checked teardown, and expanded lifecycle tests. CLI startup supports static and dynamic activation paths, while built-in plugins use explicit builtin ownership. ChangesPlugin host lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant PluginHostActivation
participant PluginRegistry
participant NativeOrWorkerRuntime
CLI->>PluginHostActivation: initialize dynamic plugin specifications
PluginHostActivation->>PluginRegistry: acquire host lease and register built-ins
PluginHostActivation->>NativeOrWorkerRuntime: load native or worker plugins
NativeOrWorkerRuntime->>PluginRegistry: register tracked plugin kinds
PluginHostActivation->>PluginRegistry: initialize owned configuration
CLI->>PluginHostActivation: clear activation during shutdown
PluginHostActivation->>PluginRegistry: clear callbacks and configuration
PluginHostActivation->>NativeOrWorkerRuntime: deregister and stop runtimes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
f73370e to
277f24e
Compare
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 902-908: Update ensure_builtin_plugins_registered and the three
builtin registration helpers to use a read-lock fast path that confirms each
expected builtin is already correctly registered, returning without acquiring
write locks when all are present. Only escalate to the existing write-lock
registration and revalidation path when a builtin is missing, while preserving
replacement rejection and retry behavior for ownership conflicts.
In `@crates/core/src/plugin/dynamic/native.rs`:
- Around line 91-116: The checked teardown loops in
crates/core/src/plugin/dynamic/native.rs:91-116 and
crates/core/src/plugin/dynamic/worker.rs:137-162 duplicate
registration-accounting logic; extract it into a shared helper such as
deregister_tracked_registrations_checked that accepts the tracked registrations
and the “native” or “worker” label, then have deregister_plugin_kinds_checked in
both activation types call that helper while preserving all existing outcomes
and error handling.
In `@crates/core/tests/unit/plugin_tests.rs`:
- Around line 1236-1310: Make the PLUGIN_MUTATION_OWNER cleanup panic-safe in
test_legacy_clear_retains_mutation_owner_after_incomplete_teardown and
test_legacy_replace_retains_mutation_owner_after_incomplete_teardown. Add an
RAII guard after acquiring the runtime-owner lock that restores the owner to
Idle on Drop, including assertion failures, and remove the manual reset
statements at the test ends.
🪄 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: c7bbf793-7860-4a03-93a5-444feae4b5b2
📒 Files selected for processing (18)
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.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Check / Run
🧰 Additional context used
📓 Path-based instructions (21)
**/*.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/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/cli/src/server.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.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/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/core/Cargo.tomlcrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.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/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.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/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/cli/src/server.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.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/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/cli/src/server.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.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/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/cli/src/server.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.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/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/cli/src/server.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.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/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/server_tests.rscrates/core/Cargo.tomlcrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/cli/src/server.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/src/plugin.rs
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/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/core/Cargo.tomlcrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.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/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/server_tests.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/cli/src/server.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.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/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/cli/tests/coverage/server_tests.rscrates/core/Cargo.tomlcrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/cli/src/server.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.rscrates/core/src/plugin.rs
crates/{core,adaptive}/**/*.rs
⚙️ CodeRabbit configuration file
crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rscrates/core/tests/unit/dynamic_worker_tests.rscrates/core/src/observability/plugin_component.rscrates/core/src/plugins/model_pricing.rscrates/core/src/plugins/nemo_guardrails/component.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/src/plugin/dynamic.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/src/plugin/dynamic/worker.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/unit/dynamic_worker_tests.rscrates/cli/tests/coverage/server_tests.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/worker_plugin_tests.rscrates/core/tests/fixtures/worker_plugin/src/main.rscrates/core/tests/unit/plugin_tests.rscrates/core/tests/integration/native_plugin_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/observability/plugin_component.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/src/plugin/dynamic.rscrates/cli/src/server.rscrates/core/src/plugin/dynamic/host.rscrates/core/src/plugin/dynamic/native.rscrates/core/src/plugin/dynamic/worker.rscrates/core/src/plugin.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/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
🔇 Additional comments (27)
crates/core/tests/unit/dynamic_worker_tests.rs (1)
1481-1481: LGTM!crates/core/tests/unit/plugin_tests.rs (5)
12-12: LGTM!Also applies to: 30-40
304-413: LGTM!Also applies to: 433-436
950-978: LGTM!
1027-1066: LGTM!Also applies to: 1096-1184
1186-1235: LGTM!crates/core/src/plugin.rs (4)
863-896: LGTM!
926-993: LGTM!
1186-1389: LGTM!
1536-1712: LGTM!Also applies to: 1738-1885
crates/core/src/plugin/dynamic.rs (1)
21-58: LGTM!crates/core/src/plugin/dynamic/native.rs (1)
118-133: LGTM!Also applies to: 145-158
crates/core/src/plugin/dynamic/worker.rs (1)
164-179: LGTM!Also applies to: 191-204, 265-424, 603-603
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/core/src/plugins/nemo_guardrails/component.rs (1)
18-18: LGTM!Also applies to: 400-402
crates/core/tests/integration/native_plugin_tests.rs (2)
8-144: LGTM!
1110-1614: LGTM!Also applies to: 1649-1657, 1776-1789
crates/core/src/plugin/dynamic/host.rs (2)
33-63: LGTM!Also applies to: 88-166, 210-425
167-196: 🩺 Stability & AvailabilityNo action needed The panic path still drops
PendingPluginRegistrations/PendingPluginRegistrationContext, so rollback failures are recorded during unwinding before this branch runs.failures.is_empty()here means there was nothing left to retain, so unloading the runtimes is safe.> Likely an incorrect or invalid review comment.crates/cli/src/server.rs (1)
149-203: LGTM!Also applies to: 317-379
crates/cli/tests/coverage/server_tests.rs (1)
1705-1734: LGTM!crates/core/Cargo.toml (1)
124-127: LGTM!crates/core/tests/fixtures/native_plugin/src/lib.rs (1)
283-298: LGTM!crates/core/tests/fixtures/worker_plugin/src/main.rs (1)
87-87: LGTM!Also applies to: 133-133, 156-159
crates/core/tests/integration/plugin_host_builtin_ownership_tests.rs (1)
19-80: LGTM!crates/core/tests/integration/worker_plugin_tests.rs (1)
26-31: LGTM!Also applies to: 48-128
mnajafian-nv
left a comment
There was a problem hiding this comment.
Thanks, this is moving in a good direction. I left two focused inline comments on validation/activation alignment and the retained-owner activity contract.
|
CLI lifecycle selection: flowchart TD
A["Relay CLI startup"] --> B{"Any plugin configuration"}
B -->|"No"| C["Run without plugin activation"]
B -->|"Yes"| D{"Dynamic plugin specs present"}
D -->|"No"| E["Use existing static initialization"]
D -->|"Yes"| F["Create PluginHostActivation"]
E --> G["Run gateway"]
F --> G
G --> H["Close sessions and flush subscribers"]
H --> I{"Activation type"}
I -->|"Static"| J["Clear static configuration"]
I -->|"Dynamic"| K["Clear dynamic host"]
|
|
Dynamic activation: sequenceDiagram
participant Caller
participant Executor as Lifecycle executor
participant Owner as Process owner
participant Loader
participant Registry
participant Config as Configuration runtime
Caller->>Executor: Activate base config and specs
Executor->>Executor: Validate nonempty and unique IDs
Executor->>Owner: Acquire exclusive host lease
Owner-->>Executor: Return owner ID
Executor->>Registry: Ensure built-in plugin kinds
Executor->>Loader: Load native libraries
Executor->>Loader: Start worker processes
Loader->>Registry: Register adapters with registration IDs
Executor->>Config: Append dynamic components after static components
Executor->>Config: Validate and initialize components
alt Initialization succeeds
Config-->>Caller: Activation guard and report
else Initialization fails cleanly
Config->>Registry: Roll back component registrations
Loader->>Registry: Deregister adapters
Loader->>Loader: Unload libraries and workers
Owner->>Owner: Release lease
Config-->>Caller: Activation error
else Rollback cannot prove safety
Loader->>Loader: Retain loaded runtimes
Owner->>Owner: Retain lease
Config-->>Caller: Incomplete rollback error
end
|
|
Safe teardown: flowchart TD
A["Clear activation"] --> B["Flush subscribers and remove component callbacks"]
B --> C{"Callbacks proven removed"}
C -->|"No"| D["Retain runtimes and owner for process lifetime"]
D --> E["Return teardown error"]
C -->|"Yes"| F["Deregister plugin adapters by registration ID"]
F --> G{"Adapters safe to unload"}
G -->|"No"| D
G -->|"Yes"| H["Shut down worker processes"]
H --> I{"Worker shutdown safe"}
I -->|"No"| D
I -->|"Yes"| J["Unload libraries and runtimes"]
J --> K["Release process owner"]
K --> L["Return success or recoverable teardown error"]
|
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
259cd55 to
84d6b1f
Compare
|
/merge |
#### Overview This adds an owned Python binding for initializing Relay configuration with explicit native and worker dynamic plugins. It builds on the shared process-wide host lifecycle merged in #364 and the discovered-configuration layering merged in #418. `initialize_with_dynamic_plugins(...)` is the owned initialization path when at least one dynamic specification is present. It resolves discovered `plugins.toml` configuration once, layers the explicit base configuration over it, then initializes statically registered components before appending dynamic-plugin components in specification order. Static-only harness-native applications continue to use `plugin.initialize()` and `plugin.clear()`. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Add `DynamicPluginActivationSpec` and async `initialize_with_dynamic_plugins(...)` to `nemo_relay.plugin`. - Return an owned `PluginHostActivation` with the validation report and activation-handle state. - Support async `close()`, `async with`, and best-effort finalization without blocking the Python finalizer or holding the GIL during teardown. - Share one cancellation-resistant close operation across repeated or concurrent callers; every caller observes the same cached result. - Document that an inactive handle has begun teardown, but does not guarantee another process-wide activation can start if teardown retained ownership for safety. - Preserve raw plugin-document omission semantics: structural `None` fields are omitted, while component-local `config` maps preserve nested `None` as JSON `null`. - Preserve the existing static `plugin.initialize()` and `plugin.clear()` APIs. Empty dynamic initialization is rejected without claiming process ownership, so static initialization remains available. - Use one Python exception-classification path for initialization and teardown failures. - Add neutral native and worker fixture coverage for callbacks, discovered/static-plus-dynamic configuration, conflicts, partial-load rollback, explicit close, context cleanup, concurrent close, cancellation, finalization, and platform-safe manifest paths. #### Where should the reviewer start? Start with `python/nemo_relay/plugin.py` for the public `initialize_with_dynamic_plugins(...)` API and `crates/python/src/py_plugin.rs` for ownership, cancellation-resistant cleanup, and exception translation. End-to-end fixture cases are in `python/tests/test_dynamic_plugin_host.py`; core discovered-configuration layering is covered by #418. #### Validation - `python/tests/test_dynamic_plugin_host.py` — 14 passed - `cargo test -p nemo-relay-python` — 63 unit tests and 1 integration test passed - `cargo clippy -p nemo-relay-python --all-targets -- -D warnings` - `cargo fmt --check` - Ruff check and format passed for changed Python files - `git diff --check` #### Related Issues Builds on #364 and #418. Authors: - Bryan Bednarski (https://github.com/bbednarski9) Approvers: - Zhongxuan (Daniel) Wang (https://github.com/zhongxuanwang-nv) - Will Killian (https://github.com/willkill07) URL: #365
#### Overview This adds an owned Node binding for explicit native and worker dynamic-plugin activation. The shared process-wide activation lifecycle landed in #364, and discovery-aware configuration layering landed in #418. This PR now contains only the Node binding and its tests. Static-only harness-native applications keep using `plugin.initialize()` / `plugin.clear()`. `initializeWithDynamicPlugins(...)` requires at least one dynamic specification. For harness-native activation, the supplied config is layered over discovered `plugins.toml` configuration; static components from that effective config initialize before components appended by the dynamic plugins. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Add `initializeWithDynamicPlugins(config, specs)` with a dedicated JavaScript/TypeScript specification shape for native and worker plugins. - Return an owned activation exposing `report`, `active`, async `close()`, and `[Symbol.asyncDispose]()` for deterministic `await using` cleanup. - Define `active` as handle state: it becomes false when teardown begins and does not by itself guarantee that process-wide ownership has been released after a teardown failure. - Share one detached close operation across repeated and concurrent callers; every caller observes the same teardown result. - Run cleanup off the JavaScript thread and retain defensive finalization when callers omit explicit close. - Preserve existing static `plugin.initialize()` and `plugin.clear()` behavior. Empty dynamic activation is rejected without claiming process ownership, so static initialization remains available. - Keep complete core validation, manifest, load, ownership, and teardown diagnostics in rejected promises. - Add neutral native and worker fixture coverage for discovered and explicit static configuration, managed LLM/tool callbacks, callback absence after close, partial-load rollback, conflicts, repeated/concurrent close, structured disposal, cancellation-safe cleanup, and garbage collection. #### Where should the reviewer start? Start with `crates/node/src/api/mod.rs` for N-API ownership and cleanup, then `crates/node/plugin.d.ts` and `crates/node/plugin.js` for the `initializeWithDynamicPlugins` public contract and deliberate absence of an old-name alias. The lifecycle cases are in `crates/node/tests/dynamic_plugin_tests.mjs`. #### Validation - `npm run build-debug` - Dynamic-plugin Node tests — 9 passed - Full Node suite — 263 passed - `npm run check:docstrings --workspace crates/node` - `cargo fmt --all -- --check` - `cargo test -p nemo-relay-node` - `cargo clippy -p nemo-relay-node --all-targets -- -D warnings` - Generated native/wrapper export consistency and stale-name checks passed - Changed-file Prettier checks passed - `git diff --check` #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) Relates to #364. Relates to #418. ## Summary by CodeRabbit - **New Features** - Added a Node.js API to activate and manage dynamically resolved plugins (`initializeWithDynamicPlugins`). - Supports native and worker dynamic plugins with per-plugin identifiers, optional environment targeting, and per-plugin JSON config. - Returns an owned activation handle exposing `active` status and a configuration validation `report`. - Provides deterministic, idempotent lifecycle teardown via `close()` and async disposal (`Symbol.asyncDispose`). - **Bug Fixes** - Improved dynamic plugin cleanup for idempotent and concurrent shutdown, including recovery after rejected activations. - **Tests** - Added comprehensive dynamic plugin host tests covering lifecycle, disposal, concurrency, and finalization behavior (native + worker). Authors: - Bryan Bednarski (https://github.com/bbednarski9) Approvers: - Zhongxuan (Daniel) Wang (https://github.com/zhongxuanwang-nv) - Will Killian (https://github.com/willkill07) URL: #366
## Summary This exposes the shared owned dynamic-plugin host through the C `nemo_relay_initialize_with_dynamic_plugins` and Go `InitializeWithDynamicPlugins` entry points for evaluation. Static-only harness-native applications continue using `nemo_relay_initialize_plugins` or Go `InitializePlugins`. Dynamic activation requires at least one specification. At startup, the binding discovers the standard `plugins.toml` files once, layers the explicit configuration over them, and then appends components loaded from the dynamic-plugin specifications. File-configured and explicitly configured static components initialize before dynamic components. Configuration files are not watched or reloaded. - expose the shared dynamic-plugin host through `nemo_relay_initialize_with_dynamic_plugins`, returning an opaque C ABI activation handle - return the validation report as caller-released JSON while keeping all library and worker ownership in Rust - layer explicit configuration over one-time `plugins.toml` discovery with explicit values taking precedence - require explicit resolved manifest paths for dynamic plugins; installation-state and manifest discovery remain outside the binding - provide sequentially idempotent clear/free functions with null and pointer-to-pointer safety - serialize concurrent C clear calls, while requiring callers to synchronize free against every use of the raw activation handle - report a C clear failure only on the call that performs teardown because the activation is consumed regardless of outcome - provide a Go `PluginActivation` wrapper with synchronous explicit `Close` and asynchronous defensive finalization - make copied Go activation values share one cleanup state and the same cached close result - reject nil or empty dynamic specifications before crossing CGo; the C API returns `InvalidArg` with actionable guidance - preserve the existing public Go config structs and JSON behavior while using a private activation wire shape so disabled static components remain disabled - preserve thread-local FFI diagnostics across Go/CGo calls - mark the dynamic activation surface as experimental in Rust docs, the generated C header, Go docs, and binding READMEs ## Reviewer guidance - The entry points are named as initialization APIs because they perform one-shot discovered, static, and dynamic component initialization. - This revision is naming-only: lifecycle behavior, validation, layering, cleanup, and ownership are unchanged. - The old C and Go names are intentionally not retained as aliases because this experimental API has not shipped. ## Validation - native and worker dynamic plugins load through the C ABI and Go/CGo wrapper - a static component discovered from a project `plugins.toml` and an explicitly supplied dynamic plugin activate together and both callbacks execute - explicit configuration overrides discovered values, static components initialize before dynamic components, and discovery occurs only once at startup - callbacks are unavailable after close - valid-first/invalid-second activation rolls back without leaking kinds or callbacks - asynchronous defensive finalization releases process-wide ownership without blocking Go's finalizer goroutine - empty specifications, invalid JSON, missing manifests, repeated sequential clear/free, copied or concurrent Go close, cached teardown errors, null cleanup, and incomplete FFI outputs are covered - public Go config JSON remains backward-compatible while activation serializes disabled static components explicitly - `cargo test -p nemo-relay-ffi --all-features` — 79 unit and 74 integration tests passed - `cargo clippy --workspace --all-targets -- -D warnings` - full `go test -v ./...` suite passed across all Go binding packages - focused finalizer and parser tests passed 20 repetitions under the Go race detector - `go vet ./...`, formatting, generated-header synchronization, and pre-commit checks passed ## Production gate Before removing the **DO NOT MERGE** label, identify a real consumer and validate at minimum: host startup/shutdown integration, activation-handle ownership, in-flight callback teardown, failure diagnostics, packaging/linking, and repeated process lifecycle behavior. ## Stack - Depends on #418 for one-time harness-native `plugins.toml` discovery and layering - Builds on the shared activation host merged through #364 ## Summary by CodeRabbit * **New Features** * Added experimental dynamic plugin activation in the FFI and Go bindings, including an opaque activation handle and JSON activation reports. * Introduced new lifecycle APIs to clear and free activation handles. * Enabled layering discovered `plugins.toml` configuration with explicitly provided activation specs. * **Bug Fixes** * Improved robustness of activation teardown (idempotent clear/free and safer cleanup after failures). * Added stricter validation for empty/invalid specs and malformed inputs. * **Documentation** * Added prominent “not production-ready” warnings for the experimental activation lifecycle. * **Tests** * Added extensive Rust and Go unit/integration tests covering activation, layering, rollback, cleanup, and finalization behavior. Authors: - Bryan Bednarski (https://github.com/bbednarski9) Approvers: - Will Killian (https://github.com/willkill07) URL: #368
Overview
This promotes dynamic-plugin activation from CLI-private orchestration into a core-owned lifecycle for harness-native embedding hosts. It deliberately does not replace the existing static plugin lifecycle: static-only callers continue to use normal plugin initialization, while
PluginHostActivationrequires at least one explicit dynamic plugin specification.Lifecycle contract
initialize_plugins*/clear_plugin_configurationremain the static-only API.PluginHostActivation::activate(base_config, dynamic_specs)is the owned dynamic API and rejects an emptydynamic_specslist with an actionable error.Details
PluginHostActivationandDynamicPluginActivationSpecAPIs for native and worker plugins.clear()plus best-effortDropcleanup that logs failures.Where should the reviewer start?
Start with
crates/core/src/plugin/dynamic/host.rsfor the public lifecycle and executor contract, thencrates/core/src/plugin.rsfor ownership and transactional rollback.crates/cli/src/server.rsshows the static/dynamic dispatch. Native and worker integration tests cover mixed static/dynamic activation, conflicts, rollback, in-flight callbacks during teardown, cancellation, explicit clear, and drop cleanup.Validation
cargo fmt --all -- --checkcargo clippy -p nemo-relay --features worker-grpc --all-targets -- -D warningscargo clippy -p nemo-relay-cli --all-targets -- -D warningsRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
None.
Summary by CodeRabbit