refactor(guardrails)!: remove built-in integration - #1172
Open
afourniernv wants to merge 1 commit into
Open
afourniernv wants to merge 1 commit into
afourniernv wants to merge 1 commit into
Conversation
Signed-off-by: Alex Fournier <afournier@nvidia.com>
github-actions
Bot
added
size:XXL
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Repository guideline files applied to this review (7)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/NeMo-Relay/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (39)
💤 Files with no reviewable changes (24)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (11)Review documentation for technical accuracy against the current API, command correctness, and consistency across language bindings.⚙️ CodeRabbit configuration file Files:
Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.⚙️ CodeRabbit configuration file Files:
Treat binding changes as public API changes.⚙️ CodeRabbit configuration file Files:
Source excerpt: In MDX files, top-of-file comments must use JSX comment delimiters: `{/*` to open and `*/}` to close.📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md) Files:
Source excerpt: Verify MDX files use JSX delimiters for top-of-file SPDX comments.📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md) Files:
Source excerpt: Search documentation source for references to the old version and update current-version install commands, package examples, and configuration examples to `` where appropriate: Review matches before changing th...📄 CodeRabbit inference engine (.agents/skills/prepare-code-freeze/SKILL.md) Files:
Source excerpt: Update `docs/about-nemo-relay/release-notes/index.mdx` unless the release changes its route or navigation entry.📄 CodeRabbit inference engine (.agents/skills/draft-release-notes/SKILL.md) Files:
Source excerpt: Python surfaces use PEP 440 translations where required; Cargo, npm, and plugin manifests use the repository SemVer form.📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md) Files:
Source excerpt: Rust `Cargo.toml` package names and workspace metadata📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md) Files:
Source excerpt: Preserve MDX front matter and the JSX SPDX comment.📄 CodeRabbit inference engine (.agents/skills/draft-release-notes/SKILL.md) Files:
Source excerpt: Consumer-facing NeMo Relay usage skills live in top-level `skills/` and are maintained independently for integrators and end users.📄 CodeRabbit inference engine (.agents/skills/README.md) Files:
🔇 Additional comments (15)
WalkthroughThe built-in NeMo Guardrails component, its local and remote backends, public configuration types, CLI editor support, and Cargo feature are removed. Static legacy entries now produce a migration diagnostic. Documentation and plugin recommendations are updated. ChangesNeMo Guardrails removal
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Other Merge Risk: ⚪ Minimal · up to The removal is accompanied by migration diagnostics and documentation. No concrete merge-blocking issue is established; merge after normal checks, with users required to remove legacy configuration entries. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 69.23% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 3 files. (12 skipped: 12 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
2 tasks done
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
willkill07
added a commit
that referenced
this pull request
Oct 1, 2026
#### Overview Passes invocation-scoped codec context to every non-streaming and streaming LLM execution interceptor across Relay core, language bindings, native plugins, and gRPC workers. Relay remains the codec owner. An interceptor can identify and use the request codec selected for the invocation, safely decode and re-encode the request, and—on non-streaming calls—decode the completed downstream response. Streaming receives request codec access only because Relay does not yet have a complete-response contract for provider chunks. - [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 - Adds one execution context argument across the existing execution-interceptor surfaces; it does not add another middleware stage or change ordering. - Carries host-owned codec operations through in-process callbacks, the native plugin ABI, language bindings, and gRPC workers. - Preserves the current request-interceptor contract and keeps complete-response decoding unavailable for streaming calls. #### Why Execution interceptors wrap the provider call but currently receive only provider JSON. A consumer that needs structured request or response data must maintain provider-specific adapters, guess the wire format, or depend on Relay's full runtime package. Those approaches duplicate host logic, can drift from Relay, and cannot cover host-owned runtime or opaque codecs. This change exposes the codec already selected by Relay through the existing execution interceptor. It does not add another middleware stage or change ordering. The NeMo Guardrails worker is the first consumer ([NVIDIA/NeMo-Relay-Plugins#6](NVIDIA/NeMo-Relay-Plugins#6)), but the API is generic. #### API contract `LlmExecutionContext` is passed immediately before `next`: | Surface | Callback shape | |---|---| | Rust core and Rust language binding | `(name, request, context, next)` | | Python language binding | `(name, request, context, next)` | | Node.js language binding | `(request, context, next)` | | Go language binding | `(request, context, next)` | | Public C API | `(user_data, name, request, context, next, next_ctx)` | | Native Rust plugin SDK | `(name, request, context, next)` for typed/async wrappers; equivalent context in raw callbacks | | Rust and Python worker SDKs | `(name, request, context, next)` | The context is directional: - `request_codec`: `LlmSanitizeRequestContext`, with identity plus decode and encode operations whenever Relay resolved a codec; - `response_codec`: optional `LlmSanitizeResponseContext`, with identity plus decode for non-streaming execution; - streaming `response_codec`: unavailable rather than offering best-effort chunk decoding. Execution intercepts reuse the existing `LlmSanitizeRequestContext` and `LlmSanitizeResponseContext` types; no parallel generic aliases are added. The native plugin SDK keeps distinct execution codec contexts because its borrowed ABI capabilities have different ownership and lifetime rules. The shared codec capability exposes identity and operations; using it does not run sanitizer middleware. Identity distinguishes absent, built-in, runtime, and opaque codecs. An opaque codec still exposes operations when Relay has the resolved codec object. Request intercepts are unchanged; `annotated_request` remains their normalized input. #### Compatibility This is an intentional source and binary break for consumers that register execution-interceptor callbacks. - Native execution callbacks move to internal ABI v7. ABI v6 remains the frozen operational-logging layout. - Native manifests continue to use `compat.native_api = "1"`. - Worker manifests continue to use `compat.worker_protocol = "grpc-v1"`; `LlmInvocation` gains an additive execution-context field. - Native plugins must rebuild against the v7 layout and set a Relay lower bound of `>=0.10.0` or another range that excludes 0.9. - Workers that register LLM execution intercepts must regenerate or upgrade their SDK, adopt the context argument, and use the same Relay compatibility floor. - Rust, Python, Node.js, Go, and C consumers using these callbacks must update to the new argument order when they adopt this Relay release. The host rejects native ABI v2-v6 layouts before callback registration. This keeps early 0.10-alpha v6 artifacts, whose callback layout lacks execution context, distinguishable from the current ABI instead of invoking them through an incompatible function signature. Keeping `native_api = "1"` and `grpc-v1` is deliberate: those labels identify the authored plugin and protocol families, while the Relay version range communicates the release-level callback break. #### Notes for reviewers - The changes under `crates/core/src/plugins/nemo_guardrails` only keep the deprecated built-in integration compiling with the new callback signature. They are not a redesign of that plugin and should disappear when removal PR [#1172](#1172) lands. Replacement source is tracked separately in [NVIDIA/NeMo-Relay-Plugins#6](NVIDIA/NeMo-Relay-Plugins#6). - Please review the native ABI path particularly closely: `crates/core/src/plugin/dynamic/native.rs`, `crates/plugin/src/lib.rs`, `crates/plugin/src/async_sdk.rs`, `crates/ffi/nemo_relay.h`, and the native fixture/tests. The important questions are table layout and versioning, rejection of stale binaries, handle ownership, retain/release balance, cancellation, and capability expiry. #### Lifetime and ownership - Core and in-process binding contexts receive revocable codec facades. Each interceptor gets an independent lease: a non-streaming lease expires when its callback settles, while a streaming request lease moves into the returned stream and expires on completion, error, close, drop, or cancellation. Retaining a facade after expiry does not keep the backing codec alive. - Worker SDKs receive proxies, never capability IDs. The host authorizes each operation against the activation and invocation that created it and revokes capabilities when non-streaming execution settles or the returned stream closes. - Safe native async wrappers retain an owning completion or stream lease. Calls after settlement fail instead of dereferencing a stale codec handle. - Streaming keeps the request capability alive through lazy polling and close, but never exposes a response decoder. - A worker stream error is terminal on both sides of the gRPC bridge. Relay closes the forwarded stream and releases continuations, scope state, active-invocation state, and codec capabilities even if the consumer retains its stream object. #### Non-goals - No response encoding API. - No generic response mutation contract beyond returning the execution interceptor's existing JSON result. - No buffering or output-guarded streaming. - No change to request intercepts, conditional middleware, cache ordering, or execution ordering. - No claim that Relay's normalized response exposes every provider candidate, tool payload, or reasoning field. #### Validation The branch covers: - request decode/encode and non-streaming response decode; - built-in, runtime, opaque, and absent codec identities; - non-streaming and lazy streaming lifetime/expiry behavior; - global and scope-local registration and callback argument order; - Rust, Python, Node.js, Go, C/FFI, native plugin, Rust worker, and Python worker surfaces; - frozen ABI-v6 logging layout, current ABI-v7 layout, and v2-v6 rejection while keeping `native_api = "1"`; - worker capability authorization, ownership, cancellation, terminal-error cleanup, and expiry while keeping `grpc-v1`. The repository CI matrix must run on the pushed head. This PR remains draft until the design is approved and that matrix is green. #### Where should the reviewer start? 1. `crates/core/src/api/runtime/llm_execution_context.rs` and `crates/core/src/api/runtime/callbacks.rs` — callback and directional context contract. 2. `crates/core/src/api/llm.rs` and the execution registry — context construction and unchanged ordering. 3. **Native ABI focus:** `crates/core/src/plugin/dynamic/native.rs`, `crates/plugin/src/lib.rs`, `crates/plugin/src/async_sdk.rs`, `crates/ffi/nemo_relay.h`, and the native fixture/tests — table layout, versioning, ownership, and stale-binary rejection. 4. `crates/worker-proto`, `crates/core/src/plugin/dynamic/worker.rs`, `crates/worker`, and `python/plugin` — invocation-scoped worker capabilities. 5. Language bindings and their global/scope-local tests. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to [RELAY-681](https://linear.app/nvidia/issue/RELAY-681/ship-an-input-only-nemo-guardrails-dynamic-python-worker) - Relates to #1172 - Consumer: [NVIDIA/NeMo-Relay-Plugins#6](NVIDIA/NeMo-Relay-Plugins#6) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * LLM execution interceptors can access the selected request codec to decode and encode requests. Non-streaming interceptors can also access the codec for completed responses; streaming interceptors do not receive response codec access. * Codec access is limited to the callback or returned stream’s lifetime. * **Compatibility** * Plugins and workers using LLM execution interceptors must update callback signatures and rebuild for Relay 0.10. Native plugins must use ABI v7; affected compatibility ranges must start at 0.10. * **Documentation** * Added codec-context guidance and Relay 0.10 migration instructions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Alex Fournier <afournier@nvidia.com> Co-authored-by: Will Killian <wkillian@nvidia.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
BREAKING CHANGE: NeMo Relay 0.10 no longer ships the built-in
nemo_guardrailscomponent, its local or remote backend, its public Rust configuration module, or theguardrails-remoteCargo feature. Legacy[[components]]entries must be removed before upgrading.Overview
Removes the built-in NeMo Guardrails integration after its deprecation in #753. This is the independently reviewed removal tracked by RELAY-692; it does not bundle or claim release of the replacement dynamic worker.
Details
[[components]] kind = "nemo_guardrails"entries with a migration-specific diagnostic. Manifest-backed dynamic plugins remain unaffected, including a future plugin that uses a Guardrails-related ID.Validation completed successfully:
just test-rust(5,308 workspace tests plus native, gRPC worker, and language-binding plugin example suites)just docsjust docs-linkcheckuv run pre-commit runcargo check -p nemo-relay --lib --all-featuresThe Fern checks passed with the expected unauthenticated warning that server-side missing-redirect validation was skipped.
Where should the reviewer start?
Start with the removed-component diagnostic in
crates/cli/src/server/mod.rs, its regression coverage incrates/cli/tests/coverage/shared/server_tests.rs, and the migration guidance indocs/reference/migration-guides.mdx.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Breaking Changes
nemo_guardrailsconfiguration entries, including disabled entries; Relay rejects them with migration guidance.guardrails-remoteCargo feature has been removed. Existing settings are not transferred automatically to another integration.Documentation