feat(plugin): add targeted LLM continuations to native C ABI v2 - #594
feat(plugin): add targeted LLM continuations to native C ABI v2#594bbednarski9 wants to merge 5 commits into
Conversation
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
WalkthroughNative plugin loading now supports manifest API versions 1 and 2. API v2 adds ABI-v4 typed unary and streaming LLM dispatch, structured failures, bounded streams, cancellation, registration APIs, exports, gateway target forwarding, and compatibility tests. ChangesNative API v2 typed dispatch
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NativePlugin
participant HostApiV4
participant Gateway
participant Provider
participant StreamCallbacks
NativePlugin->>HostApiV4: submit typed unary or streaming request
HostApiV4->>Gateway: forward target, route, headers, and method
Gateway->>Provider: send validated provider request
Provider-->>Gateway: return response, chunks, or failure
Gateway-->>HostApiV4: deliver provider result
HostApiV4-->>StreamCallbacks: invoke typed outcome or stream event
StreamCallbacks-->>NativePlugin: deliver result or terminal event
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:XL
![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/plugin/dynamic/native.rs`:
- Around line 1773-1781: Update the blocking callback error path in the
`spawn_blocking(invoke).await` handling to reclaim `completion_ref`, `next_ref`,
and the owned host string in `invocation` before propagating the `JoinError`.
Mirror the cleanup performed by the inline panic branch, while preserving the
existing `FlowError::Internal` message for the join failure.
🪄 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: 65fd9b9c-c534-4ca1-b8ec-3e7d5a62ca43
📒 Files selected for processing (8)
crates/core/src/plugin/dynamic/native.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/integration/native_plugin_tests.rscrates/core/tests/unit/native_plugin_tests.rscrates/plugin/README.mdcrates/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rsdocs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Python / Test (windows-amd64)
🧰 Additional context used
📓 Path-based instructions (33)
**/*.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/native_plugin_tests.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.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/native_plugin_tests.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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/native_plugin_tests.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/tests/integration/native_plugin_tests.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.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/native_plugin_tests.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.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/native_plugin_tests.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.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/native_plugin_tests.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.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/native_plugin_tests.rscrates/plugin/README.mddocs/build-plugins/dynamic-plugins/native-dynamic/about.mdxcrates/plugin/tests/typed_callbacks.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.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/native_plugin_tests.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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.
**/*.{rs,py,go,js,ts}: When observability configuration or lifecycle is exposed, keep FFI and Python, Go, and Node.js binding-native config objects and subscriber/exporter methods aligned in logical knobs and semantics.
Require every OpenTelemetry endpoint to have a type and nonblank destination; resolveheader_envvalues at activation and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Preserve correct handling of mark events, start/end events, orphan cases, and span or trajectory fields derived from intended event data.
Run affected Rust tests andjust test-rustwhen event fields change; runjust test-python,just test-go, andjust test-nodewhen binding-native configuration or lifecycle changes.
Files:
crates/core/tests/integration/native_plugin_tests.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/core/tests/integration/native_plugin_tests.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.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/native_plugin_tests.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/core/tests/integration/native_plugin_tests.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/fixtures/native_plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rs
**/*.{md,rst,html,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
**/*.{md,rst,html,txt}: Always spellNVIDIAin all caps. Do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun because the name starts with an 'en' sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company.
Use trademark symbols with product names only when the document type or legal guidance requires them.
Verify official capitalization, spacing, and hyphenation for product names.
Precede NVIDIA product names withNVIDIAon first mention when it is natural and accurate.
Do not rewrite product names for grammar or title-case rules.
Preserve third-party product names according to the owner's spelling.
Include the company name and full model qualifier on first use when it helps identify the model.
Preserve the official capitalization and punctuation of model names.
Use shorter family names only after the full name is established.
Spell out a term on first use and put the acronym in parentheses unless the acronym is widely understood by the intended audience.
Use the acronym on later mentions after it has been defined.
For long documents, reintroduce the full term if readers might lose context.
Form plurals of acronyms withs, not an apostrophe, such asGPUs.
In headings, common acronyms can remain abbreviated. Spell out the term in the first or second sentence of the body.
Common terms such asCPU,GPU,PC,API, andUIusually do not need to be spelled out for developer audiences.
Files:
crates/plugin/README.md
**/*.{md,rst,html}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
Link the first mention of a product name when the destination helps the reader.
Files:
crates/plugin/README.md
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
Spell
NVIDIAin all caps. Do not useNvidia,nvidia, orNV.
Files:
crates/plugin/README.md
**/*.{md,rst}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
**/*.{md,rst}: Format commands, code elements, expressions, package names, file names, and paths as inline code.
Use descriptive link text. Avoid raw URLs and weak anchors such as "here" or "read more."
Use title case consistently for technical documentation headings.
Introduce code blocks, lists, tables, and images with complete sentences.
Write procedures as imperative steps. Keep steps parallel and split long procedures into smaller tasks.
Prefer active voice, present tense, short sentences, contractions, and plain English.
Usecanfor possibility and reservemayfor permission.
Useafterfor temporal relationships instead ofonce.
Preferrefer tooverseewhen the wording points readers to another resource.
Avoid culture-specific idioms, unnecessary Latinisms, jokes, and marketing exaggeration in technical docs.
Spell out months in body text, avoid ordinal dates, and use clear time zones.
Spell out whole numbers from zero through nine unless they are technical values, parameters, versions, or UI values.
Use numerals for 10 or greater and include commas in thousands.
Do not add trademark symbols to learning-oriented docs unless the source, platform, or legal guidance explicitly requires them.
Files:
crates/plugin/README.md
**/*.md
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-technical-docs.md)
**/*.md: Use title case consistently in technical documentation headings
Avoid quotation marks, ampersands, and exclamation marks in headings
Keep product, event, research, and whitepaper names in their official title case
Use title case for table headers
Do not force social-media sentence case into technical docs
Format code elements, commands, parameters, package names, and expressions in monospace
Format directories, file names, and paths in monospace using backticks
Use angle brackets inside monospace for variables inside paths, such as/home/<username>/.login
Format error messages and strings in quotation marks, keeping literal code strings in code formatting when clearer
Format UI buttons, menus, fields, and labels in bold
Use angle brackets between UI labels for menu paths, such as File > Save As
Use italics for new terms on first use, sparingly and only when introducing the term
Use italics for publication titles
Format keyboard shortcuts in plain text, such as Press Ctrl+Alt+Delete
Use owner/repo link text for GitHub repositories, preferring[NVIDIA/NeMo](link)over prose references like 'the GitHub repo'
Introduce every code block with a complete sentence
Do not make a code block complete the grammar of the previous sentence
Do not continue a sentence after a code block
Use syntax highlighting when the format supports it for code blocks
Avoid the word 'snippet' unless the surrounding docs already use it as a term of art
Keep inline method, function, and class references consistent with nearby docs, omitting empty parentheses for prose readability when no call is shown
Use descriptive anchor text that matches the destination title when possible for links
Avoid raw URLs in running text
Avoid generic anchor text such as 'here,' 'this page,' and 'read more'
Include acronyms in link text when a linked term includes an acronym
Do not link long sentences or multiple sentences
Avoid links that pull readers away from a procedure unless the link is a p...
Files:
crates/plugin/README.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Update
README.md,fern/, package READMEs, and binding-support notes when public behavior, package names, examples, or supported bindings change.
**/*.{md,mdx}: Prefer the documented public API, not internal shortcuts
Keep package names, repo references, and build commands current
Keep release-process and release-notes guidance in repo-maintainer docs such asRELEASING.md, not as user-facing docs pages orCHANGELOG.md
Keep stable user-facing wrappers atscripts/root in docs and examples; only point at namespaced helper paths when documenting internal maintenance work
When detailed dynamic plugin guides exist, keep Rust native plugin examples, Python worker plugin examples, andgrpc-v1protocol details on separate pagesIf links in documentation change, run
just docs-linkcheck.
Files:
crates/plugin/README.mddocs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
**/*.{md,markdown,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Markdown/MDX documentation files using the HTML comment block form.
Files:
crates/plugin/README.mddocs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
{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/plugin/README.mddocs/build-plugins/dynamic-plugins/native-dynamic/about.mdxcrates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.rs
**/*.{md,mdx,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Examples and documentation must use each exporter's documented flush/deregister order before shutdown.
Files:
crates/plugin/README.mddocs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
**/*.mdx
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
MDX top-of-file SPDX comments must use {/* ... */} delimiters instead of HTML comment delimiters (Must-Fix)
In MDX files, top-of-file comments must use JSX comment delimiters (
{/*to open and*/}to close); do not use HTML comments for MDX SPDX headers
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
{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:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdxcrates/core/src/plugin/dynamic/native.rs
{docs/build-plugins/**,examples/rust-native-plugin/**,examples/python-grpc-worker-plugin/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
When detailed dynamic plugin guides exist, keep Rust native, Python worker, and
grpc-v1protocol details on separate pages.
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
{docs,examples}/**/*
📄 CodeRabbit inference engine (.agents/skills/rename-surfaces/SKILL.md)
Update docs and examples.
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
docs/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If documentation examples or commands under
docs/change, run the targeted docs checks appropriate to the change.
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
{docs/**,README.md,CONTRIBUTING.md,RELEASING.md,SECURITY.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,RELEASING.md,SECURITY.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency across language bindings.
Flag stale examples, missing SPDX headers where required, and instructions that no longer match CI or pre-commit behavior.
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
{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/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.rs
{crates/plugin/**/*.rs,python/plugin/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Rust and Python SDKs must expose every supported registration surface.
Files:
crates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.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/tests/fixtures/native_plugin/src/lib.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.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/native.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/native.rs
🧠 Learnings (1)
📚 Learning: 2026-07-28T20:07:29.880Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 571
File: crates/core/src/api/runtime/state.rs:996-1020
Timestamp: 2026-07-28T20:07:29.880Z
Learning: In NeMo Relay (RELAY-509), sanitizer callback failures must be treated as intentional fail-open behavior. When an event/tool (request/response) or LLM (request/response) sanitizer callback fails, the sanitizer chain should retain and publish the last valid event/payload snapshot (rather than dropping/invalidating the data) and log the failure including callback context (e.g., which sanitizer/callback failed and relevant identifiers). Apply this consistently across all sanitizer chains mentioned in the RELAY-509 documentation/migration guide.
Applied to files:
crates/core/src/plugin/dynamic/native.rs
🔇 Additional comments (29)
crates/plugin/src/lib.rs (9)
12-12: LGTM!Also applies to: 39-47
877-888: LGTM!
915-1087: LGTM!
1253-1324: LGTM!
2052-2059: LGTM!
2888-2935: LGTM!
3607-3629: LGTM!
3907-4013: LGTM!
4069-4099: LGTM!crates/plugin/tests/typed_callbacks.rs (1)
19-33: LGTM!Also applies to: 339-339, 374-379, 408-413, 423-441
crates/plugin/README.md (1)
34-34: LGTM!Also applies to: 52-54, 100-125
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx (1)
3-3: LGTM!Also applies to: 119-167, 169-175, 185-185
crates/core/src/plugin/dynamic/native.rs (11)
11-11: LGTM!Also applies to: 35-36, 45-72
335-342: LGTM!
389-405: LGTM!
814-829: LGTM!Also applies to: 890-890, 914-931
1433-1433: LGTM!
1535-1601: LGTM!
2295-2412: LGTM!
2415-2470: LGTM!
2626-2804: LGTM!
2806-2880: LGTM!
3136-3157: LGTM!Also applies to: 3246-3365, 3397-3458
crates/core/tests/fixtures/native_plugin/src/lib.rs (1)
300-328: LGTM!crates/core/tests/integration/native_plugin_tests.rs (1)
1201-1201: LGTM!Also applies to: 1249-1302
crates/core/tests/unit/native_plugin_tests.rs (4)
8-8: LGTM!Also applies to: 112-164
534-541: LGTM!
797-968: LGTM!
1030-1448: LGTM!
| let callback_result = if blocking { | ||
| tokio::task::spawn_blocking(invoke).await.map_err(|error| { | ||
| FlowError::Internal(format!( | ||
| "native API v2 blocking callback task failed: {error}" | ||
| )) | ||
| })? | ||
| } else { | ||
| invoke() | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Leak on the spawn_blocking join-error path.
The ? returns before reclaiming the resources this function has already leaked into raw form: completion_ref (Arc::into_raw), invocation (host string), and next_ref (Arc::into_raw). The panic branch at Lines 1784-1792 handles exactly this for the inline lane. JoinError here is reachable when the blocking pool is shut down or the task is cancelled, and the leaked next_ref/completion also retain NativeCallbackUserData, pinning the plugin library.
🛡️ Reclaim raw handles before propagating the join error
let callback_result = if blocking {
- tokio::task::spawn_blocking(invoke).await.map_err(|error| {
- FlowError::Internal(format!(
- "native API v2 blocking callback task failed: {error}"
- ))
- })?
+ match tokio::task::spawn_blocking(invoke).await {
+ Ok(result) => result,
+ Err(error) => {
+ unsafe {
+ drop(Arc::from_raw(
+ completion_ref as *const NativeAsyncCompletion,
+ ));
+ if let Some(next) = next_ref {
+ drop(Arc::from_raw(next as *const NativeAsyncNext));
+ }
+ native_string_free(invocation as *mut NemoRelayNativeString);
+ }
+ return Err(FlowError::Internal(format!(
+ "native API v2 blocking callback task failed: {error}"
+ )));
+ }
+ }
} else {
invoke()
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let callback_result = if blocking { | |
| tokio::task::spawn_blocking(invoke).await.map_err(|error| { | |
| FlowError::Internal(format!( | |
| "native API v2 blocking callback task failed: {error}" | |
| )) | |
| })? | |
| } else { | |
| invoke() | |
| }; | |
| let callback_result = if blocking { | |
| match tokio::task::spawn_blocking(invoke).await { | |
| Ok(result) => result, | |
| Err(error) => { | |
| unsafe { | |
| drop(Arc::from_raw( | |
| completion_ref as *const NativeAsyncCompletion, | |
| )); | |
| if let Some(next) = next_ref { | |
| drop(Arc::from_raw(next as *const NativeAsyncNext)); | |
| } | |
| native_string_free(invocation as *mut NemoRelayNativeString); | |
| } | |
| return Err(FlowError::Internal(format!( | |
| "native API v2 blocking callback task failed: {error}" | |
| ))); | |
| } | |
| } | |
| } else { | |
| invoke() | |
| }; |
🤖 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/plugin/dynamic/native.rs` around lines 1773 - 1781, Update
the blocking callback error path in the `spawn_blocking(invoke).await` handling
to reclaim `completion_ref`, `next_ref`, and the owned host string in
`invocation` before propagating the `JoinError`. Mirror the cleanup performed by
the inline panic branch, while preserving the existing `FlowError::Internal`
message for the join failure.
bbednarski9
changed the title
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx (1)
171-178: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a complete sentence before the code block.
Line 171 relies on the code block to complete the phrase ending in
with:. Replace it with a standalone sentence, such as “Use the following macro to export a v2-only plugin.”As per coding guidelines, “Introduce every code block with a complete sentence.”
🤖 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 `@docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx` around lines 171 - 178, Update the prose immediately before the nemo_relay_plugin_v2! code block to be a complete standalone sentence introducing the example, such as stating that the macro exports a v2-only plugin; leave the code block unchanged.Source: Coding guidelines
🤖 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/cli/tests/coverage/shared/gateway_tests.rs`:
- Around line 692-756: Update the test
typed_dispatch_target_overrides_legacy_headers_without_exposing_transport_data
to assert effective.credential_policy equals
TargetCredentialPolicy::ExplicitTarget, matching
explicit_keyless_target_drops_source_credentials. Keep the existing header and
routing assertions unchanged.
In `@crates/core/src/api/runtime/continuation_context.rs`:
- Around line 109-121: Update invoke_with_llm_dispatch_target to delegate
through the existing invoke method instead of duplicating the callback future
wrapper. Preserve the current scoping order by keeping scope_llm_dispatch_target
as the outer operation while invoke handles Relay context restoration
internally.
In `@crates/core/src/api/runtime/llm_dispatch_context.rs`:
- Around line 18-25: Replace the derived Debug implementation for
LlmDispatchTargetContext with a manual one that prints method, route, and a URL
with its query string redacted, while representing headers by names only and
never values. Preserve the existing Clone, PartialEq, and Eq derives and ensure
formatting remains available to callers of current_llm_dispatch_target.
In `@crates/core/tests/unit/native_plugin_tests.rs`:
- Around line 946-993: Strengthen the assertions in
native_api_v2_rejects_prohibited_target_methods_and_headers and
native_api_v2_rejects_target_url_credentials by checking
assert_last_error_contains with the specific validation reason after each
prepare_typed_llm_dispatch failure, not just NemoRelayStatus::InvalidArg. Use
the existing assertion pattern near line 937 and match each case to its intended
method, header, or URL-credentials error message.
In `@crates/plugin/src/lib.rs`:
- Around line 963-1037: Update LlmNonHttpFailureKindV2 with an Unknown variant
and custom Deserialize implementation that maps unrecognized serialized strings
to Unknown, while preserving existing snake_case names and known-variant
behavior. Do not rely on #[serde(other)] or add #[non_exhaustive] to the public
structs LlmHttpFailureV2 and LlmNonHttpFailureV2.
---
Outside diff comments:
In `@docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx`:
- Around line 171-178: Update the prose immediately before the
nemo_relay_plugin_v2! code block to be a complete standalone sentence
introducing the example, such as stating that the macro exports a v2-only
plugin; leave the code block unchanged.
🪄 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: 17d827ab-ebb7-4966-a303-9febc999ecdd
📒 Files selected for processing (12)
crates/cli/src/gateway/mod.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/core/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/native_plugin_tests.rscrates/plugin/README.mdcrates/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rsdocs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (35)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/core/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/cli/src/gateway/mod.rscrates/core/src/plugin/dynamic/native.rs
{crates/core,crates/adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation
Files:
crates/core/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/cli/src/gateway/mod.rscrates/core/src/plugin/dynamic/native.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/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/cli/src/gateway/mod.rscrates/core/src/plugin/dynamic/native.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/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/cli/src/gateway/mod.rscrates/core/src/plugin/dynamic/native.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/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/cli/src/gateway/mod.rscrates/core/src/plugin/dynamic/native.rs
crates/core/src/{api/**/*.rs,api/runtime/**/*.rs,codec/**/*.rs,json.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Implement the new or changed public runtime behavior first in the Rust core, especially under
crates/core/src/api/and related core modules such ascrates/core/src/api/runtime/,crates/core/src/codec/, andcrates/core/src/json.rs.
Files:
crates/core/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.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/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/cli/src/server/mod.rscrates/plugin/src/lib.rscrates/cli/src/gateway/mod.rscrates/core/src/plugin/dynamic/native.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/src/api/runtime.rsdocs/build-plugins/dynamic-plugins/native-dynamic/about.mdxcrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/cli/src/server/mod.rscrates/plugin/README.mdcrates/cli/tests/coverage/shared/gateway_tests.rscrates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/cli/src/gateway/mod.rscrates/core/src/plugin/dynamic/native.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/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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.
**/*.{rs,py,go,js,ts}: When observability configuration or lifecycle is exposed, keep FFI and Python, Go, and Node.js binding-native config objects and subscriber/exporter methods aligned in logical knobs and semantics.
Require every OpenTelemetry endpoint to have a type and nonblank destination; resolveheader_envvalues at activation and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Preserve correct handling of mark events, start/end events, orphan cases, and span or trajectory fields derived from intended event data.
Run affected Rust tests andjust test-rustwhen event fields change; runjust test-python,just test-go, andjust test-nodewhen binding-native configuration or lifecycle changes.
Files:
crates/core/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/cli/src/gateway/mod.rscrates/core/src/plugin/dynamic/native.rs
crates/core/src/api/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Preserve the documented pipeline order: conditional guardrails, request intercepts, request sanitization, execution intercepts, and response sanitization for tool and LLM execution; specialized sanitization, event creation, and dispatch for mark and scope events.
Files:
crates/core/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/core/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/tests/unit/native_plugin_tests.rscrates/cli/src/gateway/mod.rscrates/core/src/plugin/dynamic/native.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/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.rs
**/*.mdx
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
MDX top-of-file SPDX comments must use {/* ... */} delimiters instead of HTML comment delimiters (Must-Fix)
In MDX files, top-of-file comments must use JSX comment delimiters (
{/*to open and*/}to close); do not use HTML comments for MDX SPDX headers
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Update
README.md,fern/, package READMEs, and binding-support notes when public behavior, package names, examples, or supported bindings change.
**/*.{md,mdx}: Prefer the documented public API, not internal shortcuts
Keep package names, repo references, and build commands current
Keep release-process and release-notes guidance in repo-maintainer docs such asRELEASING.md, not as user-facing docs pages orCHANGELOG.md
Keep stable user-facing wrappers atscripts/root in docs and examples; only point at namespaced helper paths when documenting internal maintenance work
When detailed dynamic plugin guides exist, keep Rust native plugin examples, Python worker plugin examples, andgrpc-v1protocol details on separate pagesIf links in documentation change, run
just docs-linkcheck.
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdxcrates/plugin/README.md
**/*.{md,markdown,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Markdown/MDX documentation files using the HTML comment block form.
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdxcrates/plugin/README.md
{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:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdxcrates/plugin/README.mdcrates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.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:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdxcrates/core/src/plugin/dynamic/native.rs
{docs/build-plugins/**,examples/rust-native-plugin/**,examples/python-grpc-worker-plugin/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
When detailed dynamic plugin guides exist, keep Rust native, Python worker, and
grpc-v1protocol details on separate pages.
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
{docs,examples}/**/*
📄 CodeRabbit inference engine (.agents/skills/rename-surfaces/SKILL.md)
Update docs and examples.
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
docs/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If documentation examples or commands under
docs/change, run the targeted docs checks appropriate to the change.
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
**/*.{md,mdx,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Examples and documentation must use each exporter's documented flush/deregister order before shutdown.
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdxcrates/plugin/README.md
{docs/**,README.md,CONTRIBUTING.md,RELEASING.md,SECURITY.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,RELEASING.md,SECURITY.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency across language bindings.
Flag stale examples, missing SPDX headers where required, and instructions that no longer match CI or pre-commit behavior.
Files:
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx
**/*.{md,rst,html,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
**/*.{md,rst,html,txt}: Always spellNVIDIAin all caps. Do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun because the name starts with an 'en' sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company.
Use trademark symbols with product names only when the document type or legal guidance requires them.
Verify official capitalization, spacing, and hyphenation for product names.
Precede NVIDIA product names withNVIDIAon first mention when it is natural and accurate.
Do not rewrite product names for grammar or title-case rules.
Preserve third-party product names according to the owner's spelling.
Include the company name and full model qualifier on first use when it helps identify the model.
Preserve the official capitalization and punctuation of model names.
Use shorter family names only after the full name is established.
Spell out a term on first use and put the acronym in parentheses unless the acronym is widely understood by the intended audience.
Use the acronym on later mentions after it has been defined.
For long documents, reintroduce the full term if readers might lose context.
Form plurals of acronyms withs, not an apostrophe, such asGPUs.
In headings, common acronyms can remain abbreviated. Spell out the term in the first or second sentence of the body.
Common terms such asCPU,GPU,PC,API, andUIusually do not need to be spelled out for developer audiences.
Files:
crates/plugin/README.md
**/*.{md,rst,html}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
Link the first mention of a product name when the destination helps the reader.
Files:
crates/plugin/README.md
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
Spell
NVIDIAin all caps. Do not useNvidia,nvidia, orNV.
Files:
crates/plugin/README.md
**/*.{md,rst}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
**/*.{md,rst}: Format commands, code elements, expressions, package names, file names, and paths as inline code.
Use descriptive link text. Avoid raw URLs and weak anchors such as "here" or "read more."
Use title case consistently for technical documentation headings.
Introduce code blocks, lists, tables, and images with complete sentences.
Write procedures as imperative steps. Keep steps parallel and split long procedures into smaller tasks.
Prefer active voice, present tense, short sentences, contractions, and plain English.
Usecanfor possibility and reservemayfor permission.
Useafterfor temporal relationships instead ofonce.
Preferrefer tooverseewhen the wording points readers to another resource.
Avoid culture-specific idioms, unnecessary Latinisms, jokes, and marketing exaggeration in technical docs.
Spell out months in body text, avoid ordinal dates, and use clear time zones.
Spell out whole numbers from zero through nine unless they are technical values, parameters, versions, or UI values.
Use numerals for 10 or greater and include commas in thousands.
Do not add trademark symbols to learning-oriented docs unless the source, platform, or legal guidance explicitly requires them.
Files:
crates/plugin/README.md
**/*.md
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-technical-docs.md)
**/*.md: Use title case consistently in technical documentation headings
Avoid quotation marks, ampersands, and exclamation marks in headings
Keep product, event, research, and whitepaper names in their official title case
Use title case for table headers
Do not force social-media sentence case into technical docs
Format code elements, commands, parameters, package names, and expressions in monospace
Format directories, file names, and paths in monospace using backticks
Use angle brackets inside monospace for variables inside paths, such as/home/<username>/.login
Format error messages and strings in quotation marks, keeping literal code strings in code formatting when clearer
Format UI buttons, menus, fields, and labels in bold
Use angle brackets between UI labels for menu paths, such as File > Save As
Use italics for new terms on first use, sparingly and only when introducing the term
Use italics for publication titles
Format keyboard shortcuts in plain text, such as Press Ctrl+Alt+Delete
Use owner/repo link text for GitHub repositories, preferring[NVIDIA/NeMo](link)over prose references like 'the GitHub repo'
Introduce every code block with a complete sentence
Do not make a code block complete the grammar of the previous sentence
Do not continue a sentence after a code block
Use syntax highlighting when the format supports it for code blocks
Avoid the word 'snippet' unless the surrounding docs already use it as a term of art
Keep inline method, function, and class references consistent with nearby docs, omitting empty parentheses for prose readability when no call is shown
Use descriptive anchor text that matches the destination title when possible for links
Avoid raw URLs in running text
Avoid generic anchor text such as 'here,' 'this page,' and 'read more'
Include acronyms in link text when a linked term includes an acronym
Do not link long sentences or multiple sentences
Avoid links that pull readers away from a procedure unless the link is a p...
Files:
crates/plugin/README.md
{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/cli/tests/coverage/shared/gateway_tests.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/unit/native_plugin_tests.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/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.rs
{crates/plugin/**/*.rs,python/plugin/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Rust and Python SDKs must expose every supported registration surface.
Files:
crates/plugin/tests/typed_callbacks.rscrates/plugin/src/lib.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/native.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/native.rs
🧠 Learnings (2)
📚 Learning: 2026-07-28T20:07:29.880Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 571
File: crates/core/src/api/runtime/state.rs:996-1020
Timestamp: 2026-07-28T20:07:29.880Z
Learning: In NeMo Relay (RELAY-509), sanitizer callback failures must be treated as intentional fail-open behavior. When an event/tool (request/response) or LLM (request/response) sanitizer callback fails, the sanitizer chain should retain and publish the last valid event/payload snapshot (rather than dropping/invalidating the data) and log the failure including callback context (e.g., which sanitizer/callback failed and relevant identifiers). Apply this consistently across all sanitizer chains mentioned in the RELAY-509 documentation/migration guide.
Applied to files:
crates/core/src/api/runtime.rscrates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rscrates/core/src/plugin/dynamic/native.rs
📚 Learning: 2026-07-28T03:31:05.964Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 564
File: crates/core/src/api/runtime/subscriber_dispatcher.rs:297-314
Timestamp: 2026-07-28T03:31:05.964Z
Learning: In this codebase’s runtime API, do not implement incremental native LLM stream forwarding via the native ABI v3 asynchronous middleware protocol (it can only settle a single JSON value via a one-shot completion handle and cannot forward stream chunks incrementally). If a latency-sensitive plugin needs streaming behavior, review for use of synchronous native stream intercepts or worker plugins instead of trying to chunk-deliver or incrementally forward over the ABI v3 async path.
Applied to files:
crates/core/src/api/runtime/continuation_context.rscrates/core/src/api/runtime/llm_dispatch_context.rs
🔇 Additional comments (17)
docs/build-plugins/dynamic-plugins/native-dynamic/about.mdx (1)
174-182: 🎯 Functional CorrectnessThe documentation example is correct as written.
The
nemo_relay_plugin_v2!macro accepts any symbol name and generates a function with that name. Both native API v1 and v2 plugins can use the same symbol name (nemo_relay_register_plugin) because the safety mechanism depends on the manifest'scompat.native_apideclaration, not the symbol name. Whencompat.native_api = "2", the loader passes only the v4 host API; the generated plugin code validates the API version it receives. Symbol name reuse is safe because each plugin exists in a separate library with its own manifest configuration.> Likely an incorrect or invalid review comment.crates/plugin/src/lib.rs (1)
940-951: LGTM!Also applies to: 1039-1071
crates/plugin/README.md (1)
117-133: LGTM!crates/plugin/tests/typed_callbacks.rs (1)
38-98: LGTM!crates/core/src/api/runtime.rs (1)
9-9: LGTM!Also applies to: 27-28
crates/core/src/api/runtime/llm_dispatch_context.rs (1)
74-87: LGTM!crates/core/src/plugin/dynamic/native.rs (4)
2302-2372: LGTM!
2684-2762: LGTM!Also applies to: 2865-2913
2775-2801: 🗄️ Data Integrity & IntegrationNo target rebinding is needed in the producer task.
upstream.send().awaitcompletes beforesse_json_stream(response)constructs the returned stream. Producer polling only consumes the established response body.> Likely an incorrect or invalid review comment.
2383-2410: 🩺 Stability & AvailabilityNo cancellation arm is required
FlowErrorhas no cancellation, abort, or shutdown variant.LlmNonHttpFailureKindV2::Cancelledis emitted directly by stream lifecycle paths, not throughtyped_llm_failure.> Likely an incorrect or invalid review comment.crates/core/tests/unit/native_plugin_tests.rs (1)
131-168: LGTM!Also applies to: 815-944, 1015-1044, 1108-1320, 1323-1612
crates/cli/src/gateway/mod.rs (3)
29-30: LGTM!Also applies to: 145-145, 308-308, 317-317, 802-802, 812-823, 847-847, 875-893, 953-953, 963-970, 1169-1169
1-1: 🩺 Stability & Availability | 🏗️ Heavy liftPanic risk on malformed typed-target data is unverified and untested.
effective_dispatch_request_with_targetuses four.expect()calls that assume native-plugin-supplied header names, header values, HTTP method, and route strings were already validated. That validation is not visible in this review batch, and no test exercises the failure case.
crates/cli/src/gateway/mod.rs#L894-935: confirm (with the linked verification script) thatcrates/core/src/api/runtime/llm_dispatch_context.rsand the plugin SDK's route enum incrates/plugin/src/lib.rsfully guarantee these invariants; otherwise replace the.expect()calls with a gracefulFlowError/CliErrorpath.crates/cli/tests/coverage/shared/gateway_tests.rs#L692-756: add a test that constructs anLlmDispatchTargetContextwith an invalid header name/value, invalid method, or unrecognized route to prove the intended behavior (safe rejection, not a panic).
894-935: 🩺 Stability & AvailabilityRoute enum mismatch between plugin SDK and gateway is a latent maintenance risk; header and method validation is already enforced upstream.
LlmDispatchTargetContextcarries only untyped strings for method, route, and headers. The comments at lines 915, 917, and 926 state these values "were validated"—this is correct. Validation occurs incrates/core/src/plugin/dynamic/native.rsinprepare_typed_llm_dispatch()(lines 2302–2355) beforeLlmDispatchTargetContextis constructed:
- HTTP method (lines 2321–2328): validated via
reqwest::Method::from_bytes(), rejects CONNECT and TRACE.- Header names (lines 2330–2334): validated via
HeaderName::from_bytes().- Header values (lines 2341–2345): validated via
HeaderValue::from_str().The genuine risk is the route enum mismatch.
LlmDispatchRouteV2in the plugin SDK defines exactly three variants:OpenaiChat,OpenaiResponses,AnthropicMessages. All three map viaas_str()to strings thatProviderRoute::from_dispatch_override()accepts (lines 67, 71, 77). If the plugin SDK adds or renames a route variant without a corresponding update tofrom_dispatch_override(), the string will not match any arm, and the.expect()at line 924 panics.Enforcing sync between the plugin SDK's route enum and gateway route acceptance is the practical value. Header and method
.expect()calls are safe because validation is exhaustive upstream; the route.expect()is a maintenance invariant that requires code review discipline.crates/cli/src/server/mod.rs (1)
63-63: LGTM!Also applies to: 546-560
crates/cli/tests/coverage/shared/gateway_tests.rs (2)
16-16: LGTM!Also applies to: 2068-2068, 2107-2107
692-756: 🩺 Stability & Availability | 🏗️ Heavy liftMissing negative-path coverage for malformed dispatch targets.
This test only covers a well-formed
LlmDispatchTargetContext.effective_dispatch_request_with_targetincrates/cli/src/gateway/mod.rspanics via.expect()on invalid header names/values, an invalid method, or an unrecognized route string. Add a test constructing a target with one such invalid value to confirm the intended behavior (either a documented panic-is-unreachable invariant, backed by upstream validation, or a graceful error). This note is tied to the panic-risk finding raised oncrates/cli/src/gateway/mod.rs(lines 894-935); see the consolidated comment.As per path instructions, tests in this path should cover "error paths" for the changed API surface.
Source: Path instructions
| #[test] | ||
| fn typed_dispatch_target_overrides_legacy_headers_without_exposing_transport_data() { | ||
| let original_body = Bytes::from_static(br#"{"model":"original"}"#); | ||
| let mut original_headers = HeaderMap::new(); | ||
| original_headers.insert( | ||
| header::AUTHORIZATION, | ||
| HeaderValue::from_static("Bearer source-secret"), | ||
| ); | ||
| original_headers.insert("x-source", HeaderValue::from_static("source")); | ||
| let request = LlmRequest { | ||
| headers: Map::from_iter([ | ||
| ( | ||
| INTERNAL_DISPATCH_URL_HEADER.to_string(), | ||
| json!("http://attacker.invalid/v1/chat/completions"), | ||
| ), | ||
| ("authorization".into(), json!("Bearer request-secret")), | ||
| ("x-request".into(), json!("request")), | ||
| ]), | ||
| content: json!({"model": "selected"}), | ||
| }; | ||
| let target = LlmDispatchTargetContext::new( | ||
| "POST".into(), | ||
| "http://selected.invalid/v1/responses".into(), | ||
| "openai_responses".into(), | ||
| BTreeMap::from([ | ||
| ("authorization".into(), "Bearer target-secret".into()), | ||
| ("x-target".into(), "selected".into()), | ||
| ]), | ||
| ); | ||
|
|
||
| let effective = effective_dispatch_request_with_target( | ||
| &original_body, | ||
| &original_headers, | ||
| Some(&request), | ||
| Some(&target), | ||
| "http://default.invalid/v1/chat/completions", | ||
| &Method::GET, | ||
| ProviderRoute::OpenAiChatCompletions, | ||
| ); | ||
|
|
||
| assert_eq!(effective.method, Method::POST); | ||
| assert_eq!( | ||
| effective.url, | ||
| "http://selected.invalid/v1/responses".to_string() | ||
| ); | ||
| assert_eq!(effective.target_route, ProviderRoute::OpenAiResponses); | ||
| assert_eq!( | ||
| effective.headers.get(header::AUTHORIZATION).unwrap(), | ||
| "Bearer target-secret" | ||
| ); | ||
| assert_eq!(effective.headers.get("x-target").unwrap(), "selected"); | ||
| assert_eq!( | ||
| effective.headers.get(header::CONTENT_TYPE).unwrap(), | ||
| "application/json" | ||
| ); | ||
| assert!(effective.headers.get("x-source").is_none()); | ||
| assert!(effective.headers.get("x-request").is_none()); | ||
| assert!( | ||
| effective | ||
| .headers | ||
| .get(INTERNAL_DISPATCH_URL_HEADER) | ||
| .is_none() | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add an assertion on credential_policy for the typed-target test.
The sibling test explicit_keyless_target_drops_source_credentials asserts effective.credential_policy, but this new test does not. Add the same assertion here to confirm the typed-target branch marks the request TargetCredentialPolicy::ExplicitTarget, keeping the contract check explicit rather than implied by the absence of source headers.
As per path instructions, tests matching {crates/**/tests/**,...} should "cover the behavior promised by the changed API surface" with assertions on the relevant contract fields rather than indirect checks.
✅ Suggested assertion addition
assert_eq!(effective.target_route, ProviderRoute::OpenAiResponses);
+ assert_eq!(
+ effective.credential_policy,
+ TargetCredentialPolicy::ExplicitTarget
+ );
assert_eq!(
effective.headers.get(header::AUTHORIZATION).unwrap(),
"Bearer target-secret"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn typed_dispatch_target_overrides_legacy_headers_without_exposing_transport_data() { | |
| let original_body = Bytes::from_static(br#"{"model":"original"}"#); | |
| let mut original_headers = HeaderMap::new(); | |
| original_headers.insert( | |
| header::AUTHORIZATION, | |
| HeaderValue::from_static("Bearer source-secret"), | |
| ); | |
| original_headers.insert("x-source", HeaderValue::from_static("source")); | |
| let request = LlmRequest { | |
| headers: Map::from_iter([ | |
| ( | |
| INTERNAL_DISPATCH_URL_HEADER.to_string(), | |
| json!("http://attacker.invalid/v1/chat/completions"), | |
| ), | |
| ("authorization".into(), json!("Bearer request-secret")), | |
| ("x-request".into(), json!("request")), | |
| ]), | |
| content: json!({"model": "selected"}), | |
| }; | |
| let target = LlmDispatchTargetContext::new( | |
| "POST".into(), | |
| "http://selected.invalid/v1/responses".into(), | |
| "openai_responses".into(), | |
| BTreeMap::from([ | |
| ("authorization".into(), "Bearer target-secret".into()), | |
| ("x-target".into(), "selected".into()), | |
| ]), | |
| ); | |
| let effective = effective_dispatch_request_with_target( | |
| &original_body, | |
| &original_headers, | |
| Some(&request), | |
| Some(&target), | |
| "http://default.invalid/v1/chat/completions", | |
| &Method::GET, | |
| ProviderRoute::OpenAiChatCompletions, | |
| ); | |
| assert_eq!(effective.method, Method::POST); | |
| assert_eq!( | |
| effective.url, | |
| "http://selected.invalid/v1/responses".to_string() | |
| ); | |
| assert_eq!(effective.target_route, ProviderRoute::OpenAiResponses); | |
| assert_eq!( | |
| effective.headers.get(header::AUTHORIZATION).unwrap(), | |
| "Bearer target-secret" | |
| ); | |
| assert_eq!(effective.headers.get("x-target").unwrap(), "selected"); | |
| assert_eq!( | |
| effective.headers.get(header::CONTENT_TYPE).unwrap(), | |
| "application/json" | |
| ); | |
| assert!(effective.headers.get("x-source").is_none()); | |
| assert!(effective.headers.get("x-request").is_none()); | |
| assert!( | |
| effective | |
| .headers | |
| .get(INTERNAL_DISPATCH_URL_HEADER) | |
| .is_none() | |
| ); | |
| } | |
| #[test] | |
| fn typed_dispatch_target_overrides_legacy_headers_without_exposing_transport_data() { | |
| let original_body = Bytes::from_static(br#"{"model":"original"}"#); | |
| let mut original_headers = HeaderMap::new(); | |
| original_headers.insert( | |
| header::AUTHORIZATION, | |
| HeaderValue::from_static("Bearer source-secret"), | |
| ); | |
| original_headers.insert("x-source", HeaderValue::from_static("source")); | |
| let request = LlmRequest { | |
| headers: Map::from_iter([ | |
| ( | |
| INTERNAL_DISPATCH_URL_HEADER.to_string(), | |
| json!("http://attacker.invalid/v1/chat/completions"), | |
| ), | |
| ("authorization".into(), json!("Bearer request-secret")), | |
| ("x-request".into(), json!("request")), | |
| ]), | |
| content: json!({"model": "selected"}), | |
| }; | |
| let target = LlmDispatchTargetContext::new( | |
| "POST".into(), | |
| "http://selected.invalid/v1/responses".into(), | |
| "openai_responses".into(), | |
| BTreeMap::from([ | |
| ("authorization".into(), "Bearer target-secret".into()), | |
| ("x-target".into(), "selected".into()), | |
| ]), | |
| ); | |
| let effective = effective_dispatch_request_with_target( | |
| &original_body, | |
| &original_headers, | |
| Some(&request), | |
| Some(&target), | |
| "http://default.invalid/v1/chat/completions", | |
| &Method::GET, | |
| ProviderRoute::OpenAiChatCompletions, | |
| ); | |
| assert_eq!(effective.method, Method::POST); | |
| assert_eq!( | |
| effective.url, | |
| "http://selected.invalid/v1/responses".to_string() | |
| ); | |
| assert_eq!(effective.target_route, ProviderRoute::OpenAiResponses); | |
| assert_eq!( | |
| effective.credential_policy, | |
| TargetCredentialPolicy::ExplicitTarget | |
| ); | |
| assert_eq!( | |
| effective.headers.get(header::AUTHORIZATION).unwrap(), | |
| "Bearer target-secret" | |
| ); | |
| assert_eq!(effective.headers.get("x-target").unwrap(), "selected"); | |
| assert_eq!( | |
| effective.headers.get(header::CONTENT_TYPE).unwrap(), | |
| "application/json" | |
| ); | |
| assert!(effective.headers.get("x-source").is_none()); | |
| assert!(effective.headers.get("x-request").is_none()); | |
| assert!( | |
| effective | |
| .headers | |
| .get(INTERNAL_DISPATCH_URL_HEADER) | |
| .is_none() | |
| ); | |
| } |
🤖 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/cli/tests/coverage/shared/gateway_tests.rs` around lines 692 - 756,
Update the test
typed_dispatch_target_overrides_legacy_headers_without_exposing_transport_data
to assert effective.credential_policy equals
TargetCredentialPolicy::ExplicitTarget, matching
explicit_keyless_target_drops_source_credentials. Keep the existing header and
routing assertions unchanged.
Source: Path instructions
| /// Invoke a callback and poll its future with the captured Relay context and typed LLM target. | ||
| #[doc(hidden)] | ||
| pub async fn invoke_with_llm_dispatch_target<C, F>( | ||
| &self, | ||
| target: LlmDispatchTargetContext, | ||
| callback: C, | ||
| ) -> F::Output | ||
| where | ||
| C: FnOnce() -> F, | ||
| F: Future, | ||
| { | ||
| scope_llm_dispatch_target(target, self.run(async move { callback().await })).await | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Reuse invoke instead of duplicating its body.
invoke_with_llm_dispatch_target repeats the async move { callback().await } wrapper from invoke (Lines 128-134). Delegate instead, so the two paths cannot drift.
The scoping order stays the same: the dispatch target remains the outer task-local, and run restores the Relay context inside it.
♻️ Proposed refactor
- scope_llm_dispatch_target(target, self.run(async move { callback().await })).await
+ scope_llm_dispatch_target(target, self.invoke(callback)).await📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Invoke a callback and poll its future with the captured Relay context and typed LLM target. | |
| #[doc(hidden)] | |
| pub async fn invoke_with_llm_dispatch_target<C, F>( | |
| &self, | |
| target: LlmDispatchTargetContext, | |
| callback: C, | |
| ) -> F::Output | |
| where | |
| C: FnOnce() -> F, | |
| F: Future, | |
| { | |
| scope_llm_dispatch_target(target, self.run(async move { callback().await })).await | |
| } | |
| /// Invoke a callback and poll its future with the captured Relay context and typed LLM target. | |
| #[doc(hidden)] | |
| pub async fn invoke_with_llm_dispatch_target<C, F>( | |
| &self, | |
| target: LlmDispatchTargetContext, | |
| callback: C, | |
| ) -> F::Output | |
| where | |
| C: FnOnce() -> F, | |
| F: Future, | |
| { | |
| scope_llm_dispatch_target(target, self.invoke(callback)).await | |
| } |
🤖 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/api/runtime/continuation_context.rs` around lines 109 - 121,
Update invoke_with_llm_dispatch_target to delegate through the existing invoke
method instead of duplicating the callback future wrapper. Preserve the current
scoping order by keeping scope_llm_dispatch_target as the outer operation while
invoke handles Relay context restoration internally.
| #[doc(hidden)] | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct LlmDispatchTargetContext { | ||
| method: String, | ||
| url: String, | ||
| route: String, | ||
| headers: BTreeMap<String, String>, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact header values in Debug; the derive can leak target credentials.
headers holds plugin-supplied provider credentials. The public contract in this same PR states the opposite guarantee. crates/plugin/src/lib.rs documents for LlmDispatchTargetV2.headers: "Relay validates and transports these headers but never records their values in plugin diagnostics or observability events." crates/plugin/README.md Lines 131-133 repeat that guarantee.
The derived Debug defeats that guarantee. Any tracing field, format!("{target:?}"), expect/unwrap message, or assert_eq! failure on this type prints Authorization values verbatim. current_llm_dispatch_target is pub (hidden), so call sites outside this module can format the value.
Implement Debug manually and print header names only. Consider redacting the URL query string as well, because some providers accept API keys as query parameters.
🔒️ Proposed fix: manual redacting `Debug`
#[doc(hidden)]
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, PartialEq, Eq)]
pub struct LlmDispatchTargetContext {
method: String,
url: String,
route: String,
headers: BTreeMap<String, String>,
}
+
+impl std::fmt::Debug for LlmDispatchTargetContext {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.debug_struct("LlmDispatchTargetContext")
+ .field("method", &self.method)
+ .field("url", &self.url)
+ .field("route", &self.route)
+ .field("header_names", &self.headers.keys().collect::<Vec<_>>())
+ .finish()
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[doc(hidden)] | |
| #[derive(Clone, Debug, PartialEq, Eq)] | |
| pub struct LlmDispatchTargetContext { | |
| method: String, | |
| url: String, | |
| route: String, | |
| headers: BTreeMap<String, String>, | |
| } | |
| #[doc(hidden)] | |
| #[derive(Clone, PartialEq, Eq)] | |
| pub struct LlmDispatchTargetContext { | |
| method: String, | |
| url: String, | |
| route: String, | |
| headers: BTreeMap<String, String>, | |
| } | |
| impl std::fmt::Debug for LlmDispatchTargetContext { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| f.debug_struct("LlmDispatchTargetContext") | |
| .field("method", &self.method) | |
| .field("url", &self.url) | |
| .field("route", &self.route) | |
| .field("header_names", &self.headers.keys().collect::<Vec<_>>()) | |
| .finish() | |
| } | |
| } |
🤖 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/api/runtime/llm_dispatch_context.rs` around lines 18 - 25,
Replace the derived Debug implementation for LlmDispatchTargetContext with a
manual one that prints method, route, and a URL with its query string redacted,
while representing headers by names only and never values. Preserve the existing
Clone, PartialEq, and Eq derives and ensure formatting remains available to
callers of current_llm_dispatch_target.
| #[test] | ||
| fn native_api_v2_rejects_prohibited_target_methods_and_headers() { | ||
| let request = LlmRequest { | ||
| headers: Map::new(), | ||
| content: json!({}), | ||
| }; | ||
| let mut target = test_dispatch_target( | ||
| "https://provider.example/v1/chat/completions", | ||
| nemo_relay_plugin::LlmDispatchRouteV2::OpenaiChat, | ||
| ); | ||
| target.method = "CONNECT".into(); | ||
| assert_eq!( | ||
| prepare_typed_llm_dispatch(LlmDispatchRequestV2 { | ||
| request: request.clone(), | ||
| target, | ||
| }) | ||
| .unwrap_err(), | ||
| NemoRelayStatus::InvalidArg | ||
| ); | ||
|
|
||
| let mut target = test_dispatch_target( | ||
| "https://provider.example/v1/chat/completions", | ||
| nemo_relay_plugin::LlmDispatchRouteV2::OpenaiChat, | ||
| ); | ||
| target | ||
| .headers | ||
| .insert("x-nemo-relay-internal-dispatch-url".into(), "secret".into()); | ||
| assert_eq!( | ||
| prepare_typed_llm_dispatch(LlmDispatchRequestV2 { | ||
| request: request.clone(), | ||
| target, | ||
| }) | ||
| .unwrap_err(), | ||
| NemoRelayStatus::InvalidArg | ||
| ); | ||
|
|
||
| let mut target = test_dispatch_target( | ||
| "https://provider.example/v1/chat/completions", | ||
| nemo_relay_plugin::LlmDispatchRouteV2::OpenaiChat, | ||
| ); | ||
| target | ||
| .headers | ||
| .insert("host".into(), "attacker.invalid".into()); | ||
| assert_eq!( | ||
| prepare_typed_llm_dispatch(LlmDispatchRequestV2 { request, target }).unwrap_err(), | ||
| NemoRelayStatus::InvalidArg | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert the rejection reason, not only InvalidArg.
All three sub-cases assert the same NemoRelayStatus::InvalidArg. prepare_typed_llm_dispatch returns InvalidArg for every validation failure, including URL parsing, method checks, and header checks. A regression that broke the URL branch would still make this test pass, so the test does not pin the intended validator.
Other tests in this file already pin the reason. Line 937 uses assert_last_error_contains("absolute HTTP(S) URL"). Apply the same approach here, and to native_api_v2_rejects_target_url_credentials at Lines 996-1012.
💚 Proposed test tightening
target.method = "CONNECT".into();
assert_eq!(
prepare_typed_llm_dispatch(LlmDispatchRequestV2 {
request: request.clone(),
target,
})
.unwrap_err(),
NemoRelayStatus::InvalidArg
);
+ assert_last_error_contains("method was invalid or prohibited");
let mut target = test_dispatch_target(
"https://provider.example/v1/chat/completions",
nemo_relay_plugin::LlmDispatchRouteV2::OpenaiChat,
);
target
.headers
.insert("x-nemo-relay-internal-dispatch-url".into(), "secret".into());
assert_eq!(
prepare_typed_llm_dispatch(LlmDispatchRequestV2 {
request: request.clone(),
target,
})
.unwrap_err(),
NemoRelayStatus::InvalidArg
);
+ assert_last_error_contains("host-owned or prohibited");As per path instructions: "Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/core/tests/unit/native_plugin_tests.rs` around lines 946 - 993,
Strengthen the assertions in
native_api_v2_rejects_prohibited_target_methods_and_headers and
native_api_v2_rejects_target_url_credentials by checking
assert_last_error_contains with the specific validation reason after each
prepare_typed_llm_dispatch failure, not just NemoRelayStatus::InvalidArg. Use
the existing assertion pattern near line 937 and match each case to its intended
method, header, or URL-credentials error message.
Source: Path instructions
| /// Bounded HTTP failure returned by a provider. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, serde::Deserialize)] | ||
| pub struct LlmHttpFailureV2 { | ||
| /// Provider HTTP status. | ||
| pub status: u16, | ||
| /// Bounded provider response body. | ||
| pub body: String, | ||
| /// Safe response headers with credential-bearing fields removed. | ||
| pub headers: BTreeMap<String, String>, | ||
| } | ||
|
|
||
| /// Stable non-HTTP failure classification exposed through native API v2. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, serde::Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum LlmNonHttpFailureKindV2 { | ||
| /// Provider connection could not be established or was interrupted. | ||
| Transport, | ||
| /// Provider request timed out. | ||
| Timeout, | ||
| /// The caller cancelled the operation. | ||
| Cancelled, | ||
| /// Relay rejected an invalid dispatch request. | ||
| InvalidRequest, | ||
| /// A guardrail rejected the provider call. | ||
| Guardrail, | ||
| /// Relay could not complete the operation. | ||
| Internal, | ||
| } | ||
|
|
||
| /// Bounded failure for an operation that produced no provider HTTP response. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, serde::Deserialize)] | ||
| pub struct LlmNonHttpFailureV2 { | ||
| /// Stable failure kind. | ||
| pub kind: LlmNonHttpFailureKindV2, | ||
| /// Bounded human-readable context. | ||
| pub message: String, | ||
| } | ||
|
|
||
| /// Structured LLM continuation failure exposed through native API v2. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, serde::Deserialize)] | ||
| #[serde(tag = "kind", rename_all = "snake_case")] | ||
| pub enum LlmCallFailureV2 { | ||
| /// A provider returned a non-success HTTP response. | ||
| Http { | ||
| /// Bounded HTTP failure details. | ||
| failure: LlmHttpFailureV2, | ||
| }, | ||
| /// No provider HTTP response was available. | ||
| NonHttp { | ||
| /// Bounded non-HTTP failure details. | ||
| failure: LlmNonHttpFailureV2, | ||
| }, | ||
| } | ||
|
|
||
| impl LlmCallFailureV2 { | ||
| /// Return Relay's provider-neutral retry disposition. | ||
| /// | ||
| /// The disposition is derived rather than serialized so the wire contract | ||
| /// contains only HTTP semantics and the minimal non-HTTP failure kind. | ||
| pub const fn is_retryable(&self) -> bool { | ||
| match self { | ||
| Self::Http { failure } => is_retryable_http_status_v2(failure.status), | ||
| Self::NonHttp { failure } => matches!( | ||
| failure.kind, | ||
| LlmNonHttpFailureKindV2::Transport | LlmNonHttpFailureKindV2::Timeout | ||
| ), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Return Relay's provider-neutral retry disposition for an HTTP status. | ||
| #[must_use] | ||
| pub const fn is_retryable_http_status_v2(status: u16) -> bool { | ||
| matches!(status, 408 | 425 | 429 | 500 | 502 | 503 | 504) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
serde other attribute unit variant deserialize unknown enum value requirements
💡 Result:
To deserialize an unknown enum value in Serde, the recommended approach depends on the enum representation and whether you want to capture the unknown value or simply map it to a catch-all variant. 1. Using #[serde(other)] For internally or adjacently tagged enums, you can use the #[serde(other)] attribute on a unit variant [1][2]. This variant will be deserialized whenever the input tag does not match any of the defined variants [1]. Example: #[derive(Deserialize)] #[serde(tag = "type")] enum MyEnum { VariantA, VariantB, #[serde(other)] Unknown, } 2. Capturing Unknown Values If you need to capture the unknown value (e.g., store it as a string), #[serde(other)] is typically limited to unit variants [3]. To capture the data, common patterns include: - Untagged Enums: Use #[serde(untagged)] on a variant that can hold the data (e.g., a newtype variant) [4][5][3]. - Custom Deserialization: Implement a custom Visitor or use the #[serde(from = "...")] attribute with a From implementation to handle the conversion logic manually [3]. 3. Other Considerations - The #[serde(default)] attribute is used for different purposes, such as providing default values for missing fields in internally tagged enums, and should not be confused with the requirement for handling unknown variant tags [6][7]. - For field-based deserialization (e.g., struct keys), the #[serde(deny_unknown_fields)] attribute controls whether unknown keys trigger an error, which is separate from the enum variant handling described above [4].
Citations:
- 1: https://serde.rs/variant-attrs.html
- 2: https://hkalbasi.github.io/serde-rs.github.io/variant-attrs.html
- 3: https://stackoverflow.com/questions/57469527/how-can-i-support-an-unknown-or-other-value-for-a-serde-enum
- 4: Tagged enums should support #[serde(other)] serde-rs/serde#912
- 5: https://serde.rs/enum-representations.html
- 6: Support #[serde(default)] as a variant attribute for when the tag is not present serde-rs/serde#2985
- 7: Add ability to use
#[serde(default)]to deserialize internally tagged enums when no tag is provided serde-rs/serde#2476
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- serde declarations ---'
rg -n --glob 'Cargo.toml' 'serde(\s*=|[[:space:]]*\{)|serde_json' .
printf '%s\n' '--- DTO definitions and uses ---'
rg -n -C 4 'LlmCallFailureV2|LlmNonHttpFailureKindV2|LlmHttpFailureV2|LlmNonHttpFailureV2|serde_json::from_value|from_value::<|from_value\(' crates python
printf '%s\n' '--- relevant manifest sections ---'
for f in $(rg -l --glob 'Cargo.toml' 'serde'); do
echo "### $f"
sed -n '1,180p' "$f"
doneRepository: NVIDIA/NeMo-Relay
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workspace serde dependency declarations ---'
rg -n -C 3 --glob 'Cargo.toml' '^[[:space:]]*(serde|serde_json)[[:space:]]*=' .
printf '%s\n' '--- target DTO definition ---'
rg -n -C 12 'pub (struct|enum) Llm(HttpFailureV2|NonHttpFailureKindV2|NonHttpFailureV2|CallFailureV2)|impl LlmCallFailureV2' crates/plugin/src/lib.rs
printf '%s\n' '--- target DTO references only ---'
rg -n -C 5 'Llm(CallFailureV2|NonHttpFailureKindV2|HttpFailureV2|NonHttpFailureV2)' crates/plugin crates/core crates/worker crates/ffi crates/node crates/python
printf '%s\n' '--- plugin JSON deserialization references ---'
rg -n -C 6 'serde_json::from_value|from_slice|Deserialize' crates/plugin/src crates/plugin/testsRepository: NVIDIA/NeMo-Relay
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- locked serde versions ---'
rg -n -A 8 -B 2 'name = "(serde|serde_derive|serde_json)"' Cargo.lock
printf '%s\n' '--- all serde(other) uses and enum representations ---'
rg -n -C 5 '#\[serde\(other\)\]|#\[serde\(tag|#\[serde\(untagged\)|#\[serde\(content' crates examples pythonRepository: NVIDIA/NeMo-Relay
Length of output: 21426
🌐 Web query:
Serde #[serde(other)] externally tagged enum support exact error serde 1.x
💡 Result:
The attribute #[serde(other)] is officially supported only for unit variants within internally tagged or adjacently tagged enums [1]. It is not supported for externally tagged enums [2]. When used on an externally tagged enum, the attribute does not function as a catch-all for unknown variants [3]. While using #[serde(other)] in this context may compile without an explicit error in some versions of Serde, it will fail to produce the expected behavior (i.e., it will not capture unknown variants) [3]. There is no native "exact error" raised by the compiler for this specific misconfiguration in standard Serde 1.x, which has led to long-standing community requests for it to either be fully supported or to trigger a hard compile-time error [3][2]. For handling unknown variants in externally tagged enums, the common workarounds include: 1. Implementing a custom Deserialize trait using a visitor pattern [4]. 2. Using an "untagged" variant to capture raw data [5][4]. For example: #[derive(Deserialize)] #[serde(untagged)] enum MyEnum { VariantA, Other(serde_json::Value), // Captures everything that doesn't match VariantA } 3. Using external crates like serde-enum-str for specific string-based enum deserialization scenarios [4].
Citations:
- 1: https://serde.rs/variant-attrs.html
- 2: Give
#[serde(other)]functionality in externally tagged enums and on newtype variants serde-rs/serde#2365 - 3:
#[serde(other)]and externally tagged enum serde-rs/serde#2010 - 4: https://stackoverflow.com/questions/57469527/how-can-i-support-an-unknown-or-other-value-for-a-serde-enum
- 5: Tagged enums should support #[serde(other)] serde-rs/serde#912
Add a custom fallback for unknown failure kinds.
#[serde(other)] does not catch unknown values for the externally tagged representation used by LlmNonHttpFailureKindV2. #[non_exhaustive] only affects Rust pattern matching and does not change deserialization. Implement custom Deserialize to map unknown strings to an Unknown variant. Do not apply #[non_exhaustive] to public structs because it prevents downstream struct literals.
🤖 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/plugin/src/lib.rs` around lines 963 - 1037, Update
LlmNonHttpFailureKindV2 with an Unknown variant and custom Deserialize
implementation that maps unrecognized serialized strings to Unknown, while
preserving existing snake_case names and known-variant behavior. Do not rely on
#[serde(other)] or add #[non_exhaustive] to the public structs LlmHttpFailureV2
and LlmNonHttpFailureV2.
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/plugin/README.md (1)
124-126: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSeparate normal completion from cancellation.
Document the normal path as requesting events through
DoneorFailure, then releasing the provider stream exactly once. If the caller stops before a terminal event, document calling cancellation once before releasing the stream exactly once.🤖 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/plugin/README.md` around lines 124 - 126, Update the streaming dispatch documentation to distinguish normal completion from early cancellation: request events until receiving Done or Failure, then release the provider stream exactly once; if the caller stops before a terminal event, cancel once and then release the stream exactly once.
♻️ Duplicate comments (1)
crates/core/tests/unit/native_plugin_tests.rs (1)
949-996: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPin the specific rejection reason in these validation tests.
native_api_v2_rejects_prohibited_target_methods_and_headers(Lines 949-996) andnative_api_v2_rejects_target_url_credentials(Lines 999-1015) each assert onlyNemoRelayStatus::InvalidArg.prepare_llm_continuation_invocationreturnsInvalidArgfor every validation failure: bad method, prohibited header, invalid header value, and non-absolute or credentialed URL. A regression that breaks one specific branch (for example, the header check silently passing through) still passes these tests because a different branch also producesInvalidArg.
native_api_v2_rejects_non_absolute_dispatch_targets_before_continuation(Line 939) already pins its reason withassert_last_error_contains("absolute HTTP(S) URL"). Apply the same pattern here.💚 Proposed test tightening
target.method = "CONNECT".into(); assert_eq!( prepare_llm_continuation_invocation(LlmContinuationInvocationV2 { request: request.clone(), target, }) .unwrap_err(), NemoRelayStatus::InvalidArg ); + assert_last_error_contains("method was invalid or prohibited"); let mut target = test_dispatch_target( "https://provider.example/v1/chat/completions", nemo_relay_plugin::LlmContinuationRouteV2::OpenaiChat, ); target .headers .insert("x-nemo-relay-internal-dispatch-url".into(), "secret".into()); assert_eq!( prepare_llm_continuation_invocation(LlmContinuationInvocationV2 { request: request.clone(), target, }) .unwrap_err(), NemoRelayStatus::InvalidArg ); + assert_last_error_contains("host-owned or prohibited"); let mut target = test_dispatch_target( "https://provider.example/v1/chat/completions", nemo_relay_plugin::LlmContinuationRouteV2::OpenaiChat, ); target .headers .insert("host".into(), "attacker.invalid".into()); assert_eq!( prepare_llm_continuation_invocation(LlmContinuationInvocationV2 { request, target }) .unwrap_err(), NemoRelayStatus::InvalidArg ); + assert_last_error_contains("host-owned or prohibited"); } #[test] fn native_api_v2_rejects_target_url_credentials() { let target = test_dispatch_target( "https://user:secret@provider.example/v1/chat/completions", nemo_relay_plugin::LlmContinuationRouteV2::OpenaiChat, ); assert_eq!( prepare_llm_continuation_invocation(LlmContinuationInvocationV2 { request: LlmRequest { headers: Map::new(), content: json!({}), }, target, }) .unwrap_err(), NemoRelayStatus::InvalidArg ); + assert_last_error_contains("absolute HTTP(S) URL without user info"); }Also applies to: 999-1015
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/core/tests/unit/native_plugin_tests.rs` around lines 949 - 996, Strengthen the validation tests by asserting each rejection’s specific error message, not only NemoRelayStatus::InvalidArg. In native_api_v2_rejects_prohibited_target_methods_and_headers, call the existing assert_last_error_contains pattern after each failed prepare_llm_continuation_invocation, using distinct expected text for the CONNECT method, prohibited internal-dispatch header, and host header; apply the same reason-specific assertion in native_api_v2_rejects_target_url_credentials, following native_api_v2_rejects_non_absolute_dispatch_targets_before_continuation.Source: Path instructions
🤖 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/dynamic/native.rs`:
- Around line 2330-2353: Update the target-header validation loop before
constructing LlmDispatchTargetContext to track normalized HeaderName values and
reject any case-insensitive duplicate with InvalidArg and an appropriate native
error. Keep the existing prohibited-name and invalid-value validation unchanged,
and ensure duplicates are detected before invocation.target.headers is consumed.
In `@crates/plugin/README.md`:
- Around line 113-115: Update the registration documentation in the plugin
README to present the typed v2 registration methods first, and move the *_raw
helper references into a clearly labeled advanced ABI section. Keep the existing
compat.native_api and PluginContext::host_api_v4 details accurate while
prioritizing the documented public API over raw registration helpers.
---
Outside diff comments:
In `@crates/plugin/README.md`:
- Around line 124-126: Update the streaming dispatch documentation to
distinguish normal completion from early cancellation: request events until
receiving Done or Failure, then release the provider stream exactly once; if the
caller stops before a terminal event, cancel once and then release the stream
exactly once.
---
Duplicate comments:
In `@crates/core/tests/unit/native_plugin_tests.rs`:
- Around line 949-996: Strengthen the validation tests by asserting each
rejection’s specific error message, not only NemoRelayStatus::InvalidArg. In
native_api_v2_rejects_prohibited_target_methods_and_headers, call the existing
assert_last_error_contains pattern after each failed
prepare_llm_continuation_invocation, using distinct expected text for the
CONNECT method, prohibited internal-dispatch header, and host header; apply the
same reason-specific assertion in native_api_v2_rejects_target_url_credentials,
following
native_api_v2_rejects_non_absolute_dispatch_targets_before_continuation.
🪄 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: 93f2d5b2-b3d9-4bd7-9deb-53a01cc78cb3
📒 Files selected for processing (5)
crates/core/src/plugin/dynamic/native.rscrates/core/tests/unit/native_plugin_tests.rscrates/plugin/README.mdcrates/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (28)
**/*.{md,rst,html,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
**/*.{md,rst,html,txt}: Always spellNVIDIAin all caps. Do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun because the name starts with an 'en' sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company.
Use trademark symbols with product names only when the document type or legal guidance requires them.
Verify official capitalization, spacing, and hyphenation for product names.
Precede NVIDIA product names withNVIDIAon first mention when it is natural and accurate.
Do not rewrite product names for grammar or title-case rules.
Preserve third-party product names according to the owner's spelling.
Include the company name and full model qualifier on first use when it helps identify the model.
Preserve the official capitalization and punctuation of model names.
Use shorter family names only after the full name is established.
Spell out a term on first use and put the acronym in parentheses unless the acronym is widely understood by the intended audience.
Use the acronym on later mentions after it has been defined.
For long documents, reintroduce the full term if readers might lose context.
Form plurals of acronyms withs, not an apostrophe, such asGPUs.
In headings, common acronyms can remain abbreviated. Spell out the term in the first or second sentence of the body.
Common terms such asCPU,GPU,PC,API, andUIusually do not need to be spelled out for developer audiences.
Files:
crates/plugin/README.md
**/*.{md,rst,html}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
Link the first mention of a product name when the destination helps the reader.
Files:
crates/plugin/README.md
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
Spell
NVIDIAin all caps. Do not useNvidia,nvidia, orNV.
Files:
crates/plugin/README.md
**/*.{md,rst}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
**/*.{md,rst}: Format commands, code elements, expressions, package names, file names, and paths as inline code.
Use descriptive link text. Avoid raw URLs and weak anchors such as "here" or "read more."
Use title case consistently for technical documentation headings.
Introduce code blocks, lists, tables, and images with complete sentences.
Write procedures as imperative steps. Keep steps parallel and split long procedures into smaller tasks.
Prefer active voice, present tense, short sentences, contractions, and plain English.
Usecanfor possibility and reservemayfor permission.
Useafterfor temporal relationships instead ofonce.
Preferrefer tooverseewhen the wording points readers to another resource.
Avoid culture-specific idioms, unnecessary Latinisms, jokes, and marketing exaggeration in technical docs.
Spell out months in body text, avoid ordinal dates, and use clear time zones.
Spell out whole numbers from zero through nine unless they are technical values, parameters, versions, or UI values.
Use numerals for 10 or greater and include commas in thousands.
Do not add trademark symbols to learning-oriented docs unless the source, platform, or legal guidance explicitly requires them.
Files:
crates/plugin/README.md
**/*.md
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-technical-docs.md)
**/*.md: Use title case consistently in technical documentation headings
Avoid quotation marks, ampersands, and exclamation marks in headings
Keep product, event, research, and whitepaper names in their official title case
Use title case for table headers
Do not force social-media sentence case into technical docs
Format code elements, commands, parameters, package names, and expressions in monospace
Format directories, file names, and paths in monospace using backticks
Use angle brackets inside monospace for variables inside paths, such as/home/<username>/.login
Format error messages and strings in quotation marks, keeping literal code strings in code formatting when clearer
Format UI buttons, menus, fields, and labels in bold
Use angle brackets between UI labels for menu paths, such as File > Save As
Use italics for new terms on first use, sparingly and only when introducing the term
Use italics for publication titles
Format keyboard shortcuts in plain text, such as Press Ctrl+Alt+Delete
Use owner/repo link text for GitHub repositories, preferring[NVIDIA/NeMo](link)over prose references like 'the GitHub repo'
Introduce every code block with a complete sentence
Do not make a code block complete the grammar of the previous sentence
Do not continue a sentence after a code block
Use syntax highlighting when the format supports it for code blocks
Avoid the word 'snippet' unless the surrounding docs already use it as a term of art
Keep inline method, function, and class references consistent with nearby docs, omitting empty parentheses for prose readability when no call is shown
Use descriptive anchor text that matches the destination title when possible for links
Avoid raw URLs in running text
Avoid generic anchor text such as 'here,' 'this page,' and 'read more'
Include acronyms in link text when a linked term includes an acronym
Do not link long sentences or multiple sentences
Avoid links that pull readers away from a procedure unless the link is a p...
Files:
crates/plugin/README.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Update
README.md,fern/, package READMEs, and binding-support notes when public behavior, package names, examples, or supported bindings change.
**/*.{md,mdx}: Prefer the documented public API, not internal shortcuts
Keep package names, repo references, and build commands current
Keep release-process and release-notes guidance in repo-maintainer docs such asRELEASING.md, not as user-facing docs pages orCHANGELOG.md
Keep stable user-facing wrappers atscripts/root in docs and examples; only point at namespaced helper paths when documenting internal maintenance work
When detailed dynamic plugin guides exist, keep Rust native plugin examples, Python worker plugin examples, andgrpc-v1protocol details on separate pagesIf links in documentation change, run
just docs-linkcheck.
Files:
crates/plugin/README.md
**/*.{md,markdown,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Markdown/MDX documentation files using the HTML comment block form.
Files:
crates/plugin/README.md
{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/plugin/README.mdcrates/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rscrates/core/src/plugin/dynamic/native.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/plugin/README.mdcrates/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.rs
**/*.{md,mdx,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Examples and documentation must use each exporter's documented flush/deregister order before shutdown.
Files:
crates/plugin/README.md
**/*.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/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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/plugin/src/lib.rscrates/core/src/plugin/dynamic/native.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/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rscrates/core/src/plugin/dynamic/native.rs
{crates/plugin/**/*.rs,python/plugin/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Rust and Python SDKs must expose every supported registration surface.
Files:
crates/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.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.
**/*.{rs,py,go,js,ts}: When observability configuration or lifecycle is exposed, keep FFI and Python, Go, and Node.js binding-native config objects and subscriber/exporter methods aligned in logical knobs and semantics.
Require every OpenTelemetry endpoint to have a type and nonblank destination; resolveheader_envvalues at activation and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Preserve correct handling of mark events, start/end events, orphan cases, and span or trajectory fields derived from intended event data.
Run affected Rust tests andjust test-rustwhen event fields change; runjust test-python,just test-go, andjust test-nodewhen binding-native configuration or lifecycle changes.
Files:
crates/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/plugin/src/lib.rscrates/plugin/tests/typed_callbacks.rscrates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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/plugin/tests/typed_callbacks.rscrates/core/tests/unit/native_plugin_tests.rs
{crates/core,crates/adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/tests/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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/unit/native_plugin_tests.rscrates/core/src/plugin/dynamic/native.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/native.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/native.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/native.rs
🧠 Learnings (1)
📚 Learning: 2026-07-28T20:07:29.880Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 571
File: crates/core/src/api/runtime/state.rs:996-1020
Timestamp: 2026-07-28T20:07:29.880Z
Learning: In NeMo Relay (RELAY-509), sanitizer callback failures must be treated as intentional fail-open behavior. When an event/tool (request/response) or LLM (request/response) sanitizer callback fails, the sanitizer chain should retain and publish the last valid event/payload snapshot (rather than dropping/invalidating the data) and log the failure including callback context (e.g., which sanitizer/callback failed and relevant identifiers). Apply this consistently across all sanitizer chains mentioned in the RELAY-509 documentation/migration guide.
Applied to files:
crates/core/src/plugin/dynamic/native.rs
🔇 Additional comments (8)
crates/plugin/src/lib.rs (2)
977-990: Unknown non-HTTP failure kinds still fail deserialization.
LlmNonHttpFailureKindV2has no catch-all variant. A future host that adds a kind breaks older plugins that deserializeLlmContinuationFailureV2. This was raised on the previous commit; only the doc comment on Line 984 changed.
46-47: LGTM!Also applies to: 915-961, 1004-1031, 1042-1071, 1262-1329, 2061-2067, 2897-2943, 3619-3640, 3931-3947, 3985-4005, 4082-4082
crates/plugin/tests/typed_callbacks.rs (1)
17-21: LGTM!Also applies to: 39-98, 400-484
crates/core/src/plugin/dynamic/native.rs (2)
54-61: LGTM!Also applies to: 918-918, 1538-1538, 1577-1591, 2374-2413, 2456-2470, 2473-2531, 2688-2866, 2869-2917
916-932: 📐 Maintainability & Code QualityRun the full validation matrix before handoff.
This change touches
crates/core, so the repository rules requirecargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust, and the Python, Go, and Node.js test targets. The PR description lists workspace Clippy, the documentation build, pre-commit, and full cross-language validation as still pending.As per coding guidelines: "If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js."Source: Coding guidelines
crates/plugin/README.md (2)
34-34: LGTM!Also applies to: 52-54, 100-112, 117-123, 127-133
113-115: 🗄️ Data Integrity & IntegrationKeep the Native API v2 documentation as written.
compat.native_api = "2"selects the ABI v4 host table, and the loader test covers this contract.host_api_v4()is optional only for hosts that do not provide ABI v4.> Likely an incorrect or invalid review comment.crates/core/tests/unit/native_plugin_tests.rs (1)
8-8: LGTM!Also applies to: 29-30, 112-183, 553-559, 816-893, 896-946, 1018-1047, 1110-1248, 1250-1324, 1326-1403, 1405-1486, 1488-1617
| for (name, value) in &invocation.target.headers { | ||
| let Ok(parsed_name) = reqwest::header::HeaderName::from_bytes(name.as_bytes()) else { | ||
| set_native_last_error("LLM continuation contained an invalid target header name"); | ||
| return Err(NemoRelayStatus::InvalidArg); | ||
| }; | ||
| if prohibited_target_header(&parsed_name) { | ||
| set_native_last_error(format!( | ||
| "LLM continuation target header {parsed_name} is host-owned or prohibited" | ||
| )); | ||
| return Err(NemoRelayStatus::InvalidArg); | ||
| } | ||
| if reqwest::header::HeaderValue::from_str(value).is_err() { | ||
| set_native_last_error(format!( | ||
| "LLM continuation target header {parsed_name} had an invalid value" | ||
| )); | ||
| return Err(NemoRelayStatus::InvalidArg); | ||
| } | ||
| } | ||
| let target = LlmDispatchTargetContext::new( | ||
| method.as_str().to_owned(), | ||
| url.to_string(), | ||
| invocation.target.route.as_str().into(), | ||
| invocation.target.headers, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the gateway consumes LlmDispatchTargetContext headers.
set -eu
fd -t f 'mod.rs' crates/cli/src/gateway | xargs -r rg -n -C 6 'headers|HeaderName|HeaderValue|LlmDispatchTargetContext|current_llm_dispatch_target'
rg -n -C 4 'fn headers|fn new' crates/core/src/api/runtime/llm_dispatch_context.rsRepository: NVIDIA/NeMo-Relay
Length of output: 28968
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- native validation ---'
sed -n '2280,2370p' crates/core/src/plugin/dynamic/native.rs
printf '%s\n' '--- dispatch context ---'
cat -n crates/core/src/api/runtime/llm_dispatch_context.rs | sed -n '1,90p'
printf '%s\n' '--- target-header tests and HeaderMap construction ---'
rg -n -C 8 'target.headers|target header|LlmDispatchTargetContext|effective_dispatch_request_with_target|HeaderMap::new|target_headers.insert' crates/core/src/plugin/dynamic crates/cli crates/core/src/apiRepository: NVIDIA/NeMo-Relay
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
p = Path("crates/cli/src/gateway/mod.rs")
text = p.read_text()
start = text.index("if let Some(target) = typed_target")
end = text.index(" }\n let overrides = dispatch_overrides", start)
print(text[start:end + 5])
print("--- dependency versions ---")
for line in Path("Cargo.lock").read_text().splitlines():
if re.match(r'name = "(http|reqwest)"', line):
print(line)
PYRepository: NVIDIA/NeMo-Relay
Length of output: 1412
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- native target validation ---'
sed -n '2310,2365p' crates/core/src/plugin/dynamic/native.rs
printf '%s\n' '--- target context definition ---'
sed -n '1,85p' crates/core/src/api/runtime/llm_dispatch_context.rs
printf '%s\n' '--- relevant target-header references only ---'
rg -n -C 5 --glob '*.rs' \
'native API v2 target header|prohibited_target_header|LlmDispatchTargetContext::new|target_headers.insert|for \(name, value\) in target.headers' \
crates/core crates/cli | head -n 300
printf '%s\n' '--- locked dependency versions ---'
awk '
/^name = "(http|reqwest)"$/ { show=1; print; next }
show && /^version = / { print; show=0 }
show && /^$/ { show=0 }
' Cargo.lockRepository: NVIDIA/NeMo-Relay
Length of output: 9881
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
# Read-only model of the two relevant collection semantics:
# BTreeMap iteration is lexical by key, and HeaderMap insertion is
# case-insensitive for header names with replacement on duplicate keys.
headers = {"Authorization": "first", "authorization": "second"}
effective = {}
for name in sorted(headers):
effective[name.lower()] = headers[name]
print("BTreeMap iteration:", list(sorted(headers)))
print("effective case-insensitive headers:", effective)
print("provider values for authorization:", [effective["authorization"]])
PYRepository: NVIDIA/NeMo-Relay
Length of output: 320
Reject case-insensitive duplicate target headers.
HeaderMap::insert merges case variants, so duplicate provider headers do not occur. The later BTreeMap entry silently replaces the earlier value. Reject case-insensitive duplicates during validation instead of relying on overwrite behavior.
🤖 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/plugin/dynamic/native.rs` around lines 2330 - 2353, Update
the target-header validation loop before constructing LlmDispatchTargetContext
to track normalized HeaderName values and reject any case-insensitive duplicate
with InvalidArg and an appropriate native error. Keep the existing
prohibited-name and invalid-value validation unchanged, and ensure duplicates
are detected before invocation.target.headers is consumed.
| Set `compat.native_api = "2"` in `relay-plugin.toml`. During registration, | ||
| `PluginContext::host_api_v4` exposes the C-safe targeted LLM continuation table | ||
| and the raw v2 registration helpers. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Document the typed registration API first.
This section points authors to raw v2 registration helpers. Lead with the typed v2 registration methods, and move *_raw methods to an advanced ABI section. This follows the repository rule to prefer the documented public API over internal shortcuts.
🤖 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/plugin/README.md` around lines 113 - 115, Update the registration
documentation in the plugin README to present the typed v2 registration methods
first, and move the *_raw helper references into a clearly labeled advanced ABI
section. Keep the existing compat.native_api and PluginContext::host_api_v4
details accurate while prioritizing the documented public API over raw
registration helpers.
Source: Coding guidelines
1 participant
Overview
Important
Native dynamic-plugin compatibility notice
This PR adds an opt-in targeted LLM continuation to native dynamic-plugin C ABI v2. Existing
compat.native_api = "1"plugins retain their symbols, layouts, negotiation path, and behavior without recompilation.The v2 contract is unreleased and changed during review. Plugins built against an earlier draft of this PR must rebuild against the updated
nemo-relay-pluginSDK. No released ABI is broken.Native plugins can already wrap an LLM execution and continue with modified request JSON. They cannot direct that continuation to a plugin-selected provider target and receive the provider outcome back as a reusable buffered or streaming value.
This prevents external routing and optimization plugins from implementing algorithms that make and inspect one or more LLM calls. For example, an LLM-classifier router must call a classifier model, inspect its response, call the selected weak or strong model, and then return the final response.
This PR adds that missing primitive: a native plugin can invoke Relay's captured LLM continuation against an explicit HTTP provider target and receive provider JSON, a provider stream, an HTTP failure, or a non-HTTP failure. Relay continues to perform provider dispatch inside its existing LLM middleware, scope, and observability path.
This PR does not add a generic HTTP client, redesign Tool continuations, or introduce MCP transport support.
Details
Gap in the existing continuation
The existing native continuation can replace the LLM request JSON, but the provider binding remains associated with the original gateway request. It cannot explicitly supply:
Without this contract, an external router must either perform provider calls itself—bypassing Relay's downstream middleware and observability—or transport URL, route, credentials, and retry controls through private Relay headers.
Targeted LLM continuation contract
LlmContinuationInvocationV2carries a replacementLlmRequestplus an explicitLlmContinuationTargetV2:Relay validates the target and binds it through an invocation-scoped typed context while invoking and polling the captured downstream continuation. URL, route, credentials, and retry controls are not inserted into
LlmRequest.headers, provider JSON, marks, spans, or logs.The terminal gateway callback performs the selected request using only the validated target header map. It rejects
CONNECT,TRACE, URL user information, hop-by-hop and host-owned headers, and everyx-nemo-relay-internal-*header. Targeted dispatch does not follow redirects.Buffered outcomes distinguish:
Streaming outcomes contain provider JSON events,
done, or a late non-HTTP failure. An HTTP failure is delivered before the provider stream handle opens.Retry disposition is derived by the shared SDK helper rather than serialized. HTTP 408, 425, 429, 500, 502, 503, and 504 plus transport and timeout failures are retryable. Relay does not parse provider bodies to reclassify HTTP 400 context-window errors or HTTP 404 model errors.
ABI status codes remain reserved for malformed pointers, invalid tables, invalid serialized arguments, and other ABI faults. Provider failures arrive through callback outcomes while the ABI operation itself succeeds.
flowchart LR A["Native routing plugin"] --> B["Targeted LLM continuation"] B --> C["Request JSON"] B --> D["Method + URL + protocol"] B --> E["Target headers + credentials"] C --> F["Relay remaining LLM middleware"] D --> F E --> F F --> G["Relay provider dispatch"] G --> H{"Provider outcome"} H -- "2xx JSON" --> I["Buffered result"] H -- "2xx stream" --> J["JSON event stream"] H -- "Non-2xx" --> K["HTTP status + bounded body"] H -- "No HTTP response" --> L["Non-HTTP failure kind"] I --> A J --> A K --> A L --> A X["No transport target in LlmRequest.headers"] -.-> FCompatibility and scope
compat.native_api = "1"continues through the existing registration symbol and host-table negotiation.compat.native_api = "2"requires the v2 registration symbol and targeted LLM callbacks.nemo_relay_plugin!remains the v1 export macro;nemo_relay_plugin_v2!exports a v2-only plugin.native_api = "2"is distinct from the append-onlyNemoRelayNativeHostApiV4C host-table revision (abi_version = 4).FlowError,serde_json::Value, or allocator-owned Rust strings.Relationship to Switchyard
run_streamplugin.Validation
cargo test -p nemo-relay --lib native_api_v2— 10 passed, including 256 concurrent buffered calls, 64 concurrent 100-event streams, and 64 distinct concurrent targets.cargo test -p nemo-relay-plugin --test typed_callbacks— 60 passed, including the HTTP-derived retry matrix.cargo test -p nemo-relay-cli typed_dispatch_target_overrides_legacy_headers_without_exposing_transport_data— passed.cargo fmt --all -- --check— passed.git diff --check— passed.The full Relay cross-language matrix, workspace Clippy, documentation build, and pre-commit validation remain to run on the current revision.
Where should the reviewer start?
Start with:
crates/plugin/src/lib.rsfor the serialized LLM continuation invocation, target, outcome, and retry contracts.crates/core/src/plugin/dynamic/native.rsfor target validation, continuation invocation, failure mapping, cancellation, and ownership.crates/core/src/api/runtime/llm_dispatch_context.rsfor invocation-scoped transport isolation.crates/cli/src/gateway/mod.rsfor terminal provider dispatch and credential handling.crates/core/tests/unit/native_plugin_tests.rsfor context coverage, concurrency, bounded/redacted failures, cancellation, and stress tests.The central design decision is that the new operation is a targeted LLM continuation, not a generic HTTP continuation. The highest-risk implementation areas are target-context isolation, credential handling, streaming cancellation, and provider-stream lifetime.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit