feat: add nemoguardrails plugin contract#131
Conversation
Signed-off-by: Alex Fournier <afournier@nvidia.com>
Signed-off-by: Alex Fournier <afournier@nvidia.com>
Signed-off-by: Alex Fournier <afournier@nvidia.com>
Signed-off-by: Alex Fournier <afournier@nvidia.com>
WalkthroughThis PR introduces a complete NeMo Guardrails plugin component for the core framework. It exports a new public module, defines configuration schema with remote/local backend options, implements plugin registration, adds comprehensive validation with semantic constraint checking, and includes extensive test coverage for configuration handling and error scenarios. ChangesNeMo Guardrails Plugin Component
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/nemoguardrails/plugin_component.rs`:
- Around line 365-370: Current code treats a duplicate-registration as success
by matching the error message text "already registered" from register_plugin;
instead, make the check state-based: first call
lookup_plugin(NEMO_GUARDRAILS_PLUGIN_KIND) and if it returns Some(_) immediately
return Ok(()); otherwise call register_plugin(Arc::new(NeMoGuardrailsPlugin))
and propagate any error. If altering call order is undesirable, update error
handling to match a structured PluginError variant (e.g.,
PluginError::AlreadyRegistered) instead of inspecting the message string and use
that variant in the match for idempotent 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: 0a6c1c82-1ed6-45a5-9332-098d843644c1
📒 Files selected for processing (4)
crates/core/src/lib.rscrates/core/src/nemoguardrails/mod.rscrates/core/src/nemoguardrails/plugin_component.rscrates/core/tests/unit/nemoguardrails/plugin_component_tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
**/*.rs: Runcargo fmt --allfor FFI work as it is Rust work
Runjust test-rustfor FFI validation
Runcargo clippy --workspace --all-targets -- -D warningsto enforce warnings-as-errors linting
**/*.rs: Runcargo fmt --allfor Rust code formatting
Runcargo clippy --workspace --all-targets -- -D warningsto enforce Rust linting with no warnings
Runjust test-rustas the shared-runtime build/test wrapper for Rust changesUse Rust snake_case naming convention for Rust code
**/*.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 Rust code formatting when Node changes touch Rust files
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting when Rust files changed as part of Node work
**/*.rs: Always runjust test-rustwhen any Rust code changes
Always runcargo fmt --allwhen any Rust code changes
Always runcargo clippy --workspace --all-targets -- -D warningswhen any Rust code changesIf any Rust files changed as part of the Python work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
Files:
crates/core/src/lib.rscrates/core/src/nemoguardrails/mod.rscrates/core/tests/unit/nemoguardrails/plugin_component_tests.rscrates/core/src/nemoguardrails/plugin_component.rs
**/*.{rs,go,js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Include SPDX license headers in all Rust, Go, JavaScript, and TypeScript source files using C-style comment syntax
Files:
crates/core/src/lib.rscrates/core/src/nemoguardrails/mod.rscrates/core/tests/unit/nemoguardrails/plugin_component_tests.rscrates/core/src/nemoguardrails/plugin_component.rs
**/*.{rs,py,go,js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use
SONAR_IGNORE_START/SONAR_IGNORE_ENDmarkers only for documented false positives that cannot be resolved in code; keep ignored blocks small, add explanatory comments, and require reviewer sign-off
Files:
crates/core/src/lib.rscrates/core/src/nemoguardrails/mod.rscrates/core/tests/unit/nemoguardrails/plugin_component_tests.rscrates/core/src/nemoguardrails/plugin_component.rs
**/*.{js,ts,tsx,jsx,py,rs,go,java,c,cpp,h,cc,cxx,cs,rb,php,swift,kt}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changed files must be formatted with the language-native formatter
Files:
crates/core/src/lib.rscrates/core/src/nemoguardrails/mod.rscrates/core/tests/unit/nemoguardrails/plugin_component_tests.rscrates/core/src/nemoguardrails/plugin_component.rs
crates/{core,adaptive}/**
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/src/lib.rscrates/core/src/nemoguardrails/mod.rscrates/core/tests/unit/nemoguardrails/plugin_component_tests.rscrates/core/src/nemoguardrails/plugin_component.rs
{crates/core,crates/adaptive}/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-wasm-binding/SKILL.md)
If the change touched shared runtime semantics in
crates/coreorcrates/adaptive, also usevalidate-change
Files:
crates/core/src/lib.rscrates/core/src/nemoguardrails/mod.rscrates/core/tests/unit/nemoguardrails/plugin_component_tests.rscrates/core/src/nemoguardrails/plugin_component.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
When
crates/corechanges, run the full validation matrix across Rust, Python, Go, Node.js, and WebAssembly
crates/core/**/*.rs: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths. Keep errors explicit and binding-appropriate at the wrapper layer.
Files:
crates/core/src/lib.rscrates/core/src/nemoguardrails/mod.rscrates/core/tests/unit/nemoguardrails/plugin_component_tests.rscrates/core/src/nemoguardrails/plugin_component.rs
**/*.{py,js,ts,tsx,go,rs,md}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Format changed files with the language-native formatter before the final lint/test pass
Files:
crates/core/src/lib.rscrates/core/src/nemoguardrails/mod.rscrates/core/tests/unit/nemoguardrails/plugin_component_tests.rscrates/core/src/nemoguardrails/plugin_component.rs
**/*.{rs,py,js,ts,tsx,go}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
During iteration, prefer
uv run pre-commit run --files <changed files...>for targeted validation
Files:
crates/core/src/lib.rscrates/core/src/nemoguardrails/mod.rscrates/core/tests/unit/nemoguardrails/plugin_component_tests.rscrates/core/src/nemoguardrails/plugin_component.rs
crates/{core,adaptive}/**/*.rs
⚙️ CodeRabbit configuration file
crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/core/src/lib.rscrates/core/src/nemoguardrails/mod.rscrates/core/tests/unit/nemoguardrails/plugin_component_tests.rscrates/core/src/nemoguardrails/plugin_component.rs
{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_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/unit/nemoguardrails/plugin_component_tests.rs
🔇 Additional comments (4)
crates/core/src/lib.rs (1)
64-64: LGTM!crates/core/src/nemoguardrails/mod.rs (1)
1-15: LGTM!crates/core/src/nemoguardrails/plugin_component.rs (1)
1-364: LGTM!Also applies to: 371-1105
crates/core/tests/unit/nemoguardrails/plugin_component_tests.rs (1)
1-638: LGTM!
| match register_plugin(Arc::new(NeMoGuardrailsPlugin)) { | ||
| Ok(()) => Ok(()), | ||
| Err(PluginError::RegistrationFailed(message)) | ||
| if message.contains("already registered") | ||
| && lookup_plugin(NEMO_GUARDRAILS_PLUGIN_KIND).is_some() => | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Avoid brittle duplicate-registration detection.
This path depends on matching error message text ("already registered"), which is fragile across internal error-message changes. Prefer state-based idempotency checks.
Proposed refactor
pub fn register_nemoguardrails_component() -> PluginResult<()> {
+ if lookup_plugin(NEMO_GUARDRAILS_PLUGIN_KIND).is_some() {
+ return Ok(());
+ }
+
match register_plugin(Arc::new(NeMoGuardrailsPlugin)) {
Ok(()) => Ok(()),
- Err(PluginError::RegistrationFailed(message))
- if message.contains("already registered")
- && lookup_plugin(NEMO_GUARDRAILS_PLUGIN_KIND).is_some() =>
+ Err(PluginError::RegistrationFailed(_))
+ if lookup_plugin(NEMO_GUARDRAILS_PLUGIN_KIND).is_some() =>
{
Ok(())
}
Err(err) => Err(err),
}
}🤖 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/src/nemoguardrails/plugin_component.rs` around lines 365 - 370,
Current code treats a duplicate-registration as success by matching the error
message text "already registered" from register_plugin; instead, make the check
state-based: first call lookup_plugin(NEMO_GUARDRAILS_PLUGIN_KIND) and if it
returns Some(_) immediately return Ok(()); otherwise call
register_plugin(Arc::new(NeMoGuardrailsPlugin)) and propagate any error. If
altering call order is undesirable, update error handling to match a structured
PluginError variant (e.g., PluginError::AlreadyRegistered) instead of inspecting
the message string and use that variant in the match for idempotent behavior.
|
From a design choice -- do we want this to live in the core crate or be part of a new crate? If it's part of core, we should be forward thinking and have a |
|
@willkill07 I think a separate crate makes sense if this starts adding deps to core. Right now PR 1 doesn’t. Remote mode (PR2) doesn’t either. Local mode (PR3) as far as I researched is really the Python SDK/runtime path, not just another transport aka “call a localhost server.” If we want faithful local behavior, I’d prefer a Python helper/subprocess over in-process Python embedding from Rust. So I don’t think there’s a strong dependency reason to split into a separate crate yet. If you just want it totally modularized out of core I can design it that way. |
2 participants
Overview
Adds the first core-owned
nemoguardrailsplugin contract for NeMo Flow.This PR intentionally stays at the contract layer:
adds a public typed Rust config surface for the planned first-party
nemoguardrailspluginmodels
remoteandlocalbackend modesadds validation, schema support, and editor metadata
does not auto-register the plugin yet
does not implement backend runtime behavior yet
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.
Details
crates/core/src/nemoguardrails/as the public core module for the planned Guardrails plugin surfaceComponentSpec,NeMoGuardrailsConfig, backend config sections, and manual registration helpersmode = remote | localconfig_pathconfig_yamlcolang_contentremote.endpointremote.config_idremote.config_idsinputoutputtool_inputtool_outputcontextllm_paramsoutput_varsllm_outputlogWhere should the reviewer start?
Start in
crates/core/src/nemoguardrails/plugin_component.rs.The most important design decision in this PR is that it stops at the public contract layer: it makes the Guardrails surface broad enough to avoid repeating the old narrow example-plugin shape, but it does not auto-register or implement the backend yet.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Tests