Skip to content

fix: guard FFI request intercept output pointers#284

Open
fallintoplace wants to merge 1 commit into
NVIDIA:mainfrom
fallintoplace:fix/ffi-request-intercepts-null-out
Open

fix: guard FFI request intercept output pointers#284
fallintoplace wants to merge 1 commit into
NVIDIA:mainfrom
fallintoplace:fix/ffi-request-intercepts-null-out

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Overview

The standalone FFI request intercept helpers now validate their output pointer before writing through it, so invalid C ABI callers receive NemoRelayStatus::NullPointer instead of crashing the process.

  • I confirm this contribution is my own work, or I have the right to submit it under this project's license.
  • I searched existing issues and open pull requests, and this does not duplicate existing work.

Details

  • Added early out.is_null() checks to nemo_relay_tool_request_intercepts and nemo_relay_llm_request_intercepts.
  • Set the FFI last-error message to out pointer is null before returning NemoRelayStatus::NullPointer.
  • Initialize *out to null on non-null output pointers before parsing or middleware execution, so later error paths do not leave stale output pointers.
  • Added FFI regression coverage for null output pointers and invalid JSON paths.

Validation:

  • cargo test -p nemo-relay-ffi test_ffi_helper_rejection_and_null_name_paths -- --nocapture
  • cargo test -p nemo-relay-ffi
  • cargo fmt --all --check
  • git diff --check
  • just test-rust

Where should the reviewer start?

Start with crates/ffi/src/api/mod.rs, then check crates/ffi/tests/unit/api/registry_tests.rs for the null pointer coverage.

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • N/A

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced parameter validation for API calls with improved error detection and reporting when invalid or null parameters are provided.
    • Prevents potential issues and crashes from incorrect parameter usage.
    • Provides clearer error messages to help identify configuration issues.
  • Tests

    • Strengthened test coverage for API error handling scenarios to ensure robust error path validation and proper error message generation.

Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
@fallintoplace fallintoplace requested a review from a team as a code owner June 19, 2026 18:37
@copy-pr-bot

copy-pr-bot Bot commented Jun 19, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added size:S PR is small Bug issue describes bug; PR fixes bug lang:rust PR changes/introduces Rust code labels Jun 19, 2026
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 1c4d59a9-65b6-407f-b30c-7edafe302f8a

📥 Commits

Reviewing files that changed from the base of the PR and between d5c2407 and 3e95b30.

📒 Files selected for processing (2)
  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (14)
crates/ffi/src/api/**/*.rs

📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)

crates/ffi/src/api/**/*.rs: Add or update FFI wrappers in relevant crates/ffi/src/api/*.rs modules, re-export through crates/ffi/src/api/mod.rs, and ensure generated crates/ffi/nemo_relay.h stays correct
Use nemo_relay_ prefix for C FFI function names (e.g., nemo_relay_tool_call)

Files:

  • crates/ffi/src/api/mod.rs
**/*.rs

📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)

Use snake_case naming convention for Rust identifiers (e.g., nemo_relay_tool_call)

**/*.rs: Any Rust change must run just test-rust
Any Rust change must run cargo fmt --all
Any Rust change must run cargo clippy --workspace --all-targets -- -D warnings

**/*.rs: Run cargo fmt --all for all FFI work since it is Rust work
Run just test-rust to validate FFI changes
Run cargo clippy --workspace --all-targets -- -D warnings to enforce strict linting on FFI work

When Rust files changed as part of Go work, also run cargo fmt --all, just test-rust, and cargo clippy --workspace --all-targets -- -D warnings

**/*.rs: Run cargo fmt --all when Rust files are changed as part of Node work
Run cargo clippy --workspace --all-targets -- -D warnings when Rust files are changed as part of Node work
Run just test-rust when Rust files are changed as part of Node work

**/*.rs: Run cargo fmt --all to format all Rust code
Run cargo clippy --workspace --all-targets -- -D warnings to enforce all clippy lints as errors

**/*.rs: Run cargo fmt --all when Rust files changed as part of WebAssembly work
Run cargo clippy --workspace --all-targets -- -D warnings when Rust files changed as part of WebAssembly work

**/*.rs: If any Rust code changed, always run just test-rust
If any Rust code changed, also run cargo fmt --all
If any Rust code changed, also run cargo clippy --workspace --all-targets -- -D warnings
Run Rust formatting with cargo fmt --all
Run Rust linting with cargo clippy --workspace --all-targets -- -D warnings

**/*.rs: Use cargo fmt for Rust code formatting
Run cargo clippy -- -D warnings to lint Rust code and treat all warnings as errors
Use Rust snake_case naming convention for Rust identifiers
Include SPDX license header in all Rust source files using double-slash comment syntax
Validate Rust code with uv run pre-commit run --all-files to enforce cargo fmt formatting check, cargo clippy lints, and cargo deny aud...

Files:

  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
**/{Cargo.toml,**/*.rs}

📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)

Maintain consistency between Rust package names in Cargo.toml and their actual usage across the codebase

Files:

  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
**/*.{h,hpp,c,cpp,rs}

📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)

Ensure FFI header and library naming follows consistent conventions across platform-specific builds

Files:

  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
**/*.{rs,toml}

📄 CodeRabbit inference engine (.agents/skills/rename-surfaces/SKILL.md)

Update Rust crate names and module prefixes during coordinated rename operations

Files:

  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
crates/ffi/**

📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)

Rebuild the FFI crate in release mode so the shared library and header stay in sync when making changes to crates/ffi

Files:

  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
crates/ffi/**/*.rs

📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)

If the change touched crates/ffi, also use test-ffi-surface for validation

Files:

  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
**/*.{rs,py,js,ts,tsx,jsx,go,sh,toml,yaml,yml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Keep SPDX headers on source, docs, scripts, and configuration files. The project is Apache-2.0.

Files:

  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
**/*.{rs,py,go,js,ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Follow binding naming conventions: Rust and Python use snake_case, C FFI exports prefixed nemo_relay_, Go uses PascalCase for public APIs, Node.js uses camelCase.

Files:

  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
crates/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

crates/**/*.rs: Keep async behavior on the existing tokio-based model. Bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Use Json = serde_json::Value in Rust-facing runtime APIs for JSON payload handling.

Files:

  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
crates/{python,ffi,node,wasm}/**/*

⚙️ CodeRabbit configuration file

crates/{python,ffi,node,wasm}/**/*: Treat binding changes as public API changes. Check for parity with the other language bindings, FFI ownership/lifetime safety,
callback error propagation, stable type conversion, and consistent async/stream semantics.
Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere.

Files:

  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
**

⚙️ CodeRabbit configuration file

**:

AGENTS.md

This file provides guidance to agents, including Claude Code and OpenAI Codex, when working in this repository.

Project Overview

NeMo Relay is a multi-language agent runtime framework for execution scopes, lifecycle events, middleware, plugins, and observability around tool and LLM calls. The core runtime is Rust. Primary supported bindings are Rust, Python, and Node.js. Go, WebAssembly, and the raw C FFI are experimental and source-first.

The shared runtime model is:

  1. Scope stacks decide where work belongs and which scope-local behavior is visible.
  2. Middleware registries decide what guardrails and intercepts run around managed calls.
  3. Plugins install reusable runtime behavior from configuration.
  4. Events record runtime behavior in ATOF form.
  5. Subscribers and exporters consume events in-process or export them to ATIF, OpenTelemetry, OpenInference, or other backends.

Repository Structure

The repository layout separates the Rust runtime, language bindings, documentation,
integration patches, and agent-facing skills.

crates/
  core/       # Rust core runtime crate, published as nemo-relay
  adaptive/   # Adaptive runtime primitives and plugin components
  python/     # PyO3 native extension for the Python package
  ffi/        # Raw C ABI layer used by downstream bindings such as Go
  node/       # NAPI Node.js binding and JavaScript/TypeScript entry points
  wasm/       # wasm-bindgen WebAssembly binding and JS wrappers
python/
  nemo_relay/  # Python wrapper package: scopes, tools, LLM, middleware, typed helpers, plugins, adaptive helpers
  tests/      # Python tests
go/
  nemo_relay/  # Experimental Go CGo binding and tests
fern/         # Fern documentation site
scripts/      # Stable wrappers and helper scripts; build/test/docs entry points live in justfile
third_party/  # P...

Files:

  • crates/ffi/src/api/mod.rs
  • crates/ffi/tests/unit/api/registry_tests.rs
{crates/adaptive/**/*.rs,**/*test*.{rs,py,go,ts,js},**/*adaptive*test*.{rs,py,go,ts,js},docs/plugins/adaptive/**}

📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)

Maintain documented and tested validation and report behavior for adaptive surfaces

Files:

  • crates/ffi/tests/unit/api/registry_tests.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/ffi/tests/unit/api/registry_tests.rs
🔇 Additional comments (2)
crates/ffi/src/api/mod.rs (1)

126-131: LGTM!

Also applies to: 210-215

crates/ffi/tests/unit/api/registry_tests.rs (1)

324-348: LGTM!

Also applies to: 386-403


Walkthrough

Adds null out-pointer checks to nemo_relay_tool_request_intercepts and nemo_relay_llm_request_intercepts: if out is null, the functions set a last-error message and return NemoRelayStatus::NullPointer; otherwise they initialize *out to null_mut() before proceeding. Unit tests are extended to cover both the null-pointer and invalid-JSON error paths.

Changes

Null out-pointer guard for FFI intercept functions

Layer / File(s) Summary
Null out-pointer guard and tests
crates/ffi/src/api/mod.rs, crates/ffi/tests/unit/api/registry_tests.rs
Both nemo_relay_tool_request_intercepts and nemo_relay_llm_request_intercepts check out.is_null() early, set a last-error, and return NullPointer; otherwise they initialize *out = null_mut() before continuing. Tests assert NullPointer status with the "out pointer is null" error message on null out-pointers, and InvalidJson with the out-pointer remaining null on bad JSON.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Title follows Conventional Commits format with 'fix' type, descriptive scope-like content, and concise imperative summary under 72 characters.
Description check ✅ Passed Description includes all required sections: Overview with confirmation checkboxes, Details listing specific changes and validation steps, reviewer guidance, and Related Issues section.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug issue describes bug; PR fixes bug lang:rust PR changes/introduces Rust code size:S PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant