feat(node): initialize with dynamic plugins#366
Conversation
WalkthroughAdds a Node API for explicitly activating dynamic Rust and worker plugins, returning an owned async-disposable handle with reports, active state, and deterministic teardown. N-API bindings implement lifecycle management, while tests cover routing, errors, disposal, concurrency, and garbage collection. ChangesNode dynamic plugin activation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NodeCaller
participant initializeWithDynamicPlugins
participant CorePluginHostActivation
participant DynamicPluginActivation
NodeCaller->>initializeWithDynamicPlugins: config and activation specs
initializeWithDynamicPlugins->>CorePluginHostActivation: activate converted specs
CorePluginHostActivation-->>initializeWithDynamicPlugins: activation and validation report
initializeWithDynamicPlugins-->>NodeCaller: activation handle
NodeCaller->>DynamicPluginActivation: close() or Symbol.asyncDispose
DynamicPluginActivation->>CorePluginHostActivation: asynchronous teardown
CorePluginHostActivation-->>DynamicPluginActivation: teardown completion
DynamicPluginActivation-->>NodeCaller: Promise<void>
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
AteebNoOne
left a comment
There was a problem hiding this comment.
Thanks for putting this together! I took a look at the JavaScript bindings and left a quick suggestion on the JSDoc typing to help with intellisense. Looking forward to seeing this land!
a4ecdcc to
baa5cd8
Compare
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
baa5cd8 to
bce713b
Compare
bce713b to
0931dc0
Compare
0931dc0 to
7f9ac76
Compare
mnajafian-nv
left a comment
There was a problem hiding this comment.
Thanks, this is in good shape overall. I left one Node-surface comment on the public activation-handle contract.
zhongxuanwang-nv
left a comment
There was a problem hiding this comment.
I think overall this is good. Thanks Bryan!
7f9ac76 to
11e2dd4
Compare
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/node/plugin.js`:
- Around line 84-86: Replace the broad Array<object> annotation for the specs
parameter with a dedicated JSDoc typedef that mirrors the plugin specification
shape declared in plugin.d.ts. Define the typedef near the related activation
API and reference it from the specs parameter so plain-JS consumers receive
IntelliSense and validation before native calls.
In `@crates/node/tests/dynamic_plugin_tests.mjs`:
- Around line 310-325: Update the cleanup logic surrounding the Promise.race and
process.kill calls so errors from the main test operation are not overwritten by
cleanup failures. In the outer finally block, record non-ESRCH process.kill
errors instead of throwing immediately, complete both firstClose and secondClose
cleanup, then propagate the recorded cleanup error only when no earlier error is
already being propagated.
🪄 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: caefdc9d-8feb-4902-b05c-745799c645f1
📒 Files selected for processing (4)
crates/node/plugin.d.tscrates/node/plugin.jscrates/node/src/api/mod.rscrates/node/tests/dynamic_plugin_tests.mjs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (15)
crates/node/**/*.{js,ts,jsx,tsx,json}
📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md)
Format changed Node files with
npm run format --workspace=nemo-relay-node
Files:
crates/node/plugin.d.tscrates/node/plugin.js
crates/node/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md)
Use
npm run check:docstrings --workspace=nemo-relay-nodeto validate public API docstring checks when surface docs changed
Files:
crates/node/plugin.d.ts
crates/node/**/*.{js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
camelCasefor Node.js public APIs.
Files:
crates/node/plugin.d.tscrates/node/plugin.jscrates/node/tests/dynamic_plugin_tests.mjs
**/*.{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/node/plugin.d.tscrates/node/plugin.jscrates/node/src/api/mod.rscrates/node/tests/dynamic_plugin_tests.mjs
**/*.{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/node/plugin.d.tscrates/node/plugin.jscrates/node/src/api/mod.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/node/plugin.d.tscrates/node/plugin.jscrates/node/src/api/mod.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:
crates/node/plugin.d.tscrates/node/plugin.js
**/*
📄 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/node/plugin.d.tscrates/node/plugin.jscrates/node/src/api/mod.rscrates/node/tests/dynamic_plugin_tests.mjs
**/*.{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/node/plugin.d.tscrates/node/plugin.jscrates/node/src/api/mod.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/node/plugin.d.tscrates/node/plugin.jscrates/node/src/api/mod.rscrates/node/tests/dynamic_plugin_tests.mjs
**/*.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/node/src/api/mod.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/node/src/api/mod.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:
crates/node/src/api/mod.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/node/src/api/mod.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/node/tests/dynamic_plugin_tests.mjs
🧠 Learnings (1)
📚 Learning: 2026-05-07T18:04:44.387Z
Learnt from: mnajafian-nv
Repo: NVIDIA/NeMo-Flow PR: 67
File: integrations/openclaw/src/modules.ts:1-2
Timestamp: 2026-05-07T18:04:44.387Z
Learning: In NVIDIA/NeMo-Flow, TypeScript source files should use `//` line comments for SPDX headers (e.g., `// SPDX-FileCopyrightText: ...` and `// SPDX-License-Identifier: ...`) rather than C-style block comments (`/* ... */`). The repo’s copyright checker enforces this mapping, so `//` SPDX headers in `.ts` files should not be flagged as a style violation.
Applied to files:
crates/node/plugin.d.ts
🪛 Biome (2.5.3)
crates/node/tests/dynamic_plugin_tests.mjs
[error] 321-321: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
🔇 Additional comments (7)
crates/node/plugin.js (2)
91-93: 🎯 Functional CorrectnessVerify base
plugins.tomlisn't silently skipped, matching the PR-365 pattern.A past reviewer flagged this exact spot as resembling a bug in PR-365 where a code path skipped loading the base
plugins.toml.activateDynamicPluginsrequires callers to passconfigexplicitly with no mention of default-manifest discovery; please confirm this is intentional and matches how other bindings (Python/Go) and the CLI handle the base config for dynamic activation, so Node doesn't diverge in a way that silently drops static plugins.#!/bin/bash rg -n -i 'plugins\.toml' --type-not lock -g '!target' -g '!node_modules'
178-178: LGTM!crates/node/plugin.d.ts (1)
48-72: LGTM!Also applies to: 325-340
crates/node/src/api/mod.rs (3)
49-52: LGTM!Also applies to: 4014-4038, 4149-4170, 4172-4213, 4215-4244
4014-4038: 🗄️ Data Integrity & IntegrationConfirm Python/Go binding parity for this new activation-spec surface.
NodeDynamicPluginActivationSpecintroduces a new Node-facing shape forDynamicPluginActivationSpec. This PR's file set doesn't include Python (crates/python/src/py_api/mod.rs,python/nemo_relay/**) or Go (go/nemo_relay/**) counterparts. Per the binding-parity path instruction, please confirm an equivalent surface is planned/tracked for those bindings (even if sequenced in a later stacked PR) so they don't silently diverge.As per path instructions, "Treat binding changes as public API changes. Check for parity with the other language bindings... Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere."
4084-4147: 🎯 Functional CorrectnessNo issue: this workspace targets edition 2024 and pins Rust 1.93.0, so
if log_finalizer_error && let Err(error) = &resultis supported.> Likely an incorrect or invalid review comment.crates/node/tests/dynamic_plugin_tests.mjs (1)
1-181: LGTM!Also applies to: 183-195, 197-291, 333-474
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/node/tests/dynamic_plugin_tests.mjs (1)
351-384: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMove the
throwstatements outsidefinallyto fully satisfy Biome'snoUnsafeFinally.The error-priority logic is now correct (operation error wins over cleanup error), but Biome still flags lines 380 and 383 since the throws are syntactically inside
finally. Since thetry/catchalready fully absorbs any operation error before reachingfinally, nothing else can throw past it — the twothrowstatements can be hoisted to just after thetry/finallyblock with no behavior change, clearing the lint error outright.🧹 Proposed fix to satisfy Biome without changing behavior
- if (operationFailed) { - throw operationError; - } - if (cleanupFailed) { - throw cleanupError; - } + } + if (operationFailed) { + throw operationError; + } + if (cleanupFailed) { + throw cleanupError; }🤖 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/node/tests/dynamic_plugin_tests.mjs` around lines 351 - 384, Move the operation and cleanup error throws out of the finally block in the surrounding Promise.race test flow, placing them immediately after the try/finally completes. Preserve the existing priority where operationError is thrown before cleanupError, while keeping cleanup and error capture inside finally unchanged.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@crates/node/tests/dynamic_plugin_tests.mjs`:
- Around line 351-384: Move the operation and cleanup error throws out of the
finally block in the surrounding Promise.race test flow, placing them
immediately after the try/finally completes. Preserve the existing priority
where operationError is thrown before cleanupError, while keeping cleanup and
error capture inside finally unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 65a92e1d-c046-46bb-accf-71032a24b7f6
📒 Files selected for processing (3)
crates/node/plugin.d.tscrates/node/src/api/mod.rscrates/node/tests/dynamic_plugin_tests.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
crates/node/**/*.{js,ts,jsx,tsx,json}
📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md)
Format changed Node files with
npm run format --workspace=nemo-relay-node
Files:
crates/node/plugin.d.ts
crates/node/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md)
Use
npm run check:docstrings --workspace=nemo-relay-nodeto validate public API docstring checks when surface docs changed
Files:
crates/node/plugin.d.ts
crates/node/**/*.{js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
camelCasefor Node.js public APIs.
Files:
crates/node/plugin.d.tscrates/node/tests/dynamic_plugin_tests.mjs
**/*.{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/node/plugin.d.tscrates/node/tests/dynamic_plugin_tests.mjscrates/node/src/api/mod.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/node/plugin.d.tscrates/node/src/api/mod.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/node/plugin.d.tscrates/node/src/api/mod.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:
crates/node/plugin.d.ts
**/*
📄 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/node/plugin.d.tscrates/node/tests/dynamic_plugin_tests.mjscrates/node/src/api/mod.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/node/plugin.d.tscrates/node/src/api/mod.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/node/plugin.d.tscrates/node/tests/dynamic_plugin_tests.mjscrates/node/src/api/mod.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/node/tests/dynamic_plugin_tests.mjs
**/*.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/node/src/api/mod.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/node/src/api/mod.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:
crates/node/src/api/mod.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/node/src/api/mod.rs
🧠 Learnings (1)
📚 Learning: 2026-05-07T18:04:44.387Z
Learnt from: mnajafian-nv
Repo: NVIDIA/NeMo-Flow PR: 67
File: integrations/openclaw/src/modules.ts:1-2
Timestamp: 2026-05-07T18:04:44.387Z
Learning: In NVIDIA/NeMo-Flow, TypeScript source files should use `//` line comments for SPDX headers (e.g., `// SPDX-FileCopyrightText: ...` and `// SPDX-License-Identifier: ...`) rather than C-style block comments (`/* ... */`). The repo’s copyright checker enforces this mapping, so `//` SPDX headers in `.ts` files should not be flagged as a style violation.
Applied to files:
crates/node/plugin.d.ts
🪛 Biome (2.5.3)
crates/node/tests/dynamic_plugin_tests.mjs
[error] 380-380: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
[error] 383-383: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
🔇 Additional comments (7)
crates/node/plugin.d.ts (1)
4-5: LGTM!crates/node/src/api/mod.rs (5)
93-104: LGTM! Correctly patchesSymbol.asyncDisposeonto the prototype after napi-rs registers exports, and removes the string-named shim — resolves the prior "redundantasync_disposeshim" concern (now also verified by the'[Symbol.asyncDispose]' in prototypeassertion added indynamic_plugin_tests.mjs).
4222-4230: LGTM! Doc comment now clarifies the string-named shim is purely for TS declaration generation and is stripped at runtime — addresses the earlier duplicate-shim review feedback.
4247-4268: 🗄️ Data Integrity & IntegrationVerify Python/Go binding parity for
activate_dynamic_plugins.This adds a new public dynamic-plugin-activation surface only to the Node binding. Per coding guidelines, exposed surfaces should get corresponding Python (
crates/python/src/py_api/mod.rs,python/nemo_relay/**) and Go (go/nemo_relay/**) bindings, and path instructions call for flagging bindings updated without parity elsewhere. Confirm this is intentionally Node-only for this stack (tracked in a later stacked PR) rather than a gap.As per coding guidelines, "Update the language-native bindings for every exposed surface in Python, Go, and Node.js," and per path instructions, "Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere."
Sources: Coding guidelines, Path instructions
4098-4161: LGTM! Teardown coordination (spawn + panic-catch + leak-on-spawn-failure fallback),report()/active()/close()getters, the specFromconversion, and the new imports look correct and consistent with the documented ownership/lifetime design.Also applies to: 4187-4220, 4028-4051, 24-25, 49-52
4139-4141: 🎯 Functional CorrectnessThis let-chain is supported by the workspace toolchain. The repo pins Rust 1.93.0 and edition 2024, so this syntax compiles here.
> Likely an incorrect or invalid review comment.crates/node/tests/dynamic_plugin_tests.mjs (1)
298-298: LGTM! Correctly verifies the string-named[Symbol.asyncDispose]shim was stripped at runtime.
|
Review scope follow-up: the binding-parity observations are intentional sequencing, not an omitted surface in this PR. Python dynamic activation is covered by #365 and FFI/Go by #368, both built on the shared core activation API. This PR remains scoped to the Node binding. The base |
|
/ok to test 553c2d6 |
## Overview Adds a shared harness-native activation path that layers explicit plugin configuration over the default discovered plugins.toml files before loading dynamic plugins. The existing exact activation path remains available for hosts such as the Relay CLI that already resolved their configuration. ## Details - Reuse the same one-time config resolution used by static initialization. - Add PluginHostActivation::activate_with_discovered_config for language and FFI bindings. - Keep PluginHostActivation::activate exact and free of implicit discovery. - Validate that a file-configured static component and a dynamic component activate in one owned transaction. ## Validation - cargo test -p nemo-relay --test native_plugin_integration (28 passed) - cargo clippy -p nemo-relay --all-targets -- -D warnings - cargo fmt --all -- --check - git diff --check ## Related Issues Relates to #365, #366, and #368. ## Summary by CodeRabbit * **New Features** * Added a plugin activation entrypoint that layers discovered `plugins.toml` configuration with dynamically provided plugin components. * **Refactor** * Improved configuration resolution by centralizing how default/discovered configuration is merged with provided settings before activation. * **Bug Fixes** * Ensured activation cleanup correctly deregisters the discovered static-base plugin kind after clearing, without impacting dynamic registrations. * **Tests** * Added an end-to-end integration test covering combined discovered + dynamic activation and verifying post-clear deregistration. Authors: - Bryan Bednarski (https://github.com/bbednarski9) Approvers: - Will Killian (https://github.com/willkill07) URL: #418
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
553c2d6 to
8735e6d
Compare
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
|
/merge |
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 discoveredplugins.tomlconfiguration; static components from that effective config initialize before components appended by the dynamic plugins.Details
initializeWithDynamicPlugins(config, specs)with a dedicated JavaScript/TypeScript specification shape for native and worker plugins.report,active, asyncclose(), and[Symbol.asyncDispose]()for deterministicawait usingcleanup.activeas 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.plugin.initialize()andplugin.clear()behavior. Empty dynamic activation is rejected without claiming process ownership, so static initialization remains available.Where should the reviewer start?
Start with
crates/node/src/api/mod.rsfor N-API ownership and cleanup, thencrates/node/plugin.d.tsandcrates/node/plugin.jsfor theinitializeWithDynamicPluginspublic contract and deliberate absence of an old-name alias. The lifecycle cases are incrates/node/tests/dynamic_plugin_tests.mjs.Validation
npm run build-debugnpm run check:docstrings --workspace crates/nodecargo fmt --all -- --checkcargo test -p nemo-relay-nodecargo clippy -p nemo-relay-node --all-targets -- -D warningsgit diff --checkRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to #364.
Relates to #418.
Summary by CodeRabbit
New Features
initializeWithDynamicPlugins).activestatus and a configuration validationreport.close()and async disposal (Symbol.asyncDispose).Bug Fixes
Tests