feat(codec): add OCI GenAI typed variants and response codec (1/4) - #554
feat(codec): add OCI GenAI typed variants and response codec (1/4)#554fede-kamel wants to merge 9 commits into
Conversation
github-actions
Bot
added
size:L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds OCI GenAI request and response variants, exports a codec supporting GENERIC, COHERE, and COHEREV2 response formats, maps OCI metadata into normalized responses and observability, and adds unit and pipeline integration tests. ChangesOCI GenAI codec support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OCI as OCI GenAI response
participant Codec as OCIGenAIChatCodec
participant Decoder as GENERIC, COHERE, or COHEREV2 decoder
participant Pipeline as LLM pipeline
participant Result as AnnotatedLlmResponse
OCI->>Codec: provide REST-format JSON
Codec->>Decoder: dispatch using apiFormat
Decoder-->>Codec: return content, tool calls, finish reason, and usage
Codec->>Result: populate normalized fields and OCI metadata
Pipeline->>Result: emit decoded annotated response
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
How this series fits togetherThis PR is layer 1 of 4. Each layer adds one capability, each is independently buildable and testable, and each later branch is a pure superset of the previous — so review feedback here rebases cleanly through the rest of the stack.
Branch links and the full plan of record are on #548. Lineage: #549 (Python approach, closed per review) → #552 (this same content as a single Rust PR, closed in favor of this reviewable split). Verification provenanceEvery branch in the stack passes the full local gate on its own: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/core/src/codec/oci_genai.rs`:
- Around line 124-162: Update flatten_all_text_parts to return None for an empty
parts slice, while preserving Text concatenation for non-empty arrays containing
only valid TEXT parts. This ensures decode_generic_content returns None for
content: [] and does not mark tool-call-only messages as containing text.
In `@crates/core/tests/unit/codec/oci_genai_tests.rs`:
- Around line 131-167: Add error-path coverage alongside the existing OCI GenAI
codec tests for GENERIC responses: create a test such as
test_invalid_generic_content_shape_errors that passes a numeric or object
message.content to OCIGenAIChatCodec.decode_response and asserts it returns an
error. Also cover an array containing a non-object part if practical, ensuring
decode_generic_content rejects each invalid shape with
FlowError::InvalidArgument.
🪄 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: a9645a77-4c88-4c9e-8a1c-90f1492e291a
📒 Files selected for processing (5)
crates/core/src/codec/mod.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/types/src/codec/request.rscrates/types/src/codec/response.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (17)
**/*.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_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/src/codec/mod.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.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/codec/mod.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.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/codec/mod.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.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/codec/mod.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.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/codec/mod.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.rs
{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/types/src/codec/response.rscrates/types/src/codec/request.rs
crates/types/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Runtime helpers must cover marks, scopes, continuations, and isolated scope stacks.
Files:
crates/types/src/codec/response.rscrates/types/src/codec/request.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/unit/codec/oci_genai_tests.rs
🔇 Additional comments (8)
crates/types/src/codec/request.rs (1)
537-549: LGTM!crates/types/src/codec/response.rs (1)
329-339: LGTM!crates/core/src/codec/mod.rs (1)
19-19: LGTM!crates/core/src/codec/oci_genai.rs (3)
197-254: LGTM!
262-329: LGTM!
90-101: LGTM!Also applies to: 332-341
crates/core/tests/unit/codec/oci_genai_tests.rs (2)
16-129: LGTM!
169-188: LGTM!
ccb3281 to
21d6e18
Compare
|
Both review findings addressed in the updated commit:
Full workspace build and test suite re-run green after the change; the fix is propagated through the staged follow-up branches of the series. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/core/src/codec/oci_genai.rs`:
- Around line 313-333: The decode_response_tool_call path silently discards
malformed tool calls when the name is missing or non-string. Add an appropriate
debug or warning log before returning None for this validation failure,
including enough context to identify the malformed OCI payload while preserving
the existing successful decoding behavior.
In `@crates/core/tests/unit/codec/oci_genai_tests.rs`:
- Around line 1-210: Add a test near test_invalid_generic_content_shape_errors
that decodes a GENERIC response with mixed TEXT and non-TEXT content through
OCIGenAIChatCodec::decode_response. Assert the message is MessageContent::Parts,
contains both parts, and specifically verify the non-TEXT part is represented as
ContentPart::ProviderNative with its original payload preserved.
🪄 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: 09237b3d-efcb-4116-9173-2272097f9a8c
📒 Files selected for processing (5)
crates/core/src/codec/mod.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/types/src/codec/request.rscrates/types/src/codec/response.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (17)
**/*.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/types/src/codec/response.rscrates/types/src/codec/request.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
{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/types/src/codec/response.rscrates/types/src/codec/request.rs
crates/types/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Runtime helpers must cover marks, scopes, continuations, and isolated scope stacks.
Files:
crates/types/src/codec/response.rscrates/types/src/codec/request.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/unit/codec/oci_genai_tests.rs
🔇 Additional comments (4)
crates/types/src/codec/request.rs (1)
537-549: LGTM!crates/types/src/codec/response.rs (1)
329-339: LGTM!crates/core/src/codec/oci_genai.rs (1)
1-113: LGTM!Also applies to: 201-258, 266-345
crates/core/src/codec/mod.rs (1)
19-19: LGTM!
|
End-to-end validation re-run on the updated series content (including the empty-content fix above), via the Python binding built from the stack tip:
Unit suite on the same tip: 33 OCI codec tests green, full workspace build and tests passing, clippy zero warnings. |
Introduce the first layer of the OCI Generative AI provider codec:
typed ApiSpecificRequest::OCIGenAI and ApiSpecificResponse::OCIGenAI
variants, plus an OCIGenAIChatCodec implementing LlmResponseCodec.
Response decode covers ChatResult ({modelId, chatResponse}) and bare
chat responses in both apiFormat variants (GENERIC choices-based and
COHERE text-based), tolerates camelCase, kebab-case, and snake_case
key conventions (SDK vs CLI shapes), parses GENERIC string-encoded
tool-call arguments, maps promptTokens/completionTokens/totalTokens
into Usage, and maps finish reasons (stop/COMPLETE -> complete,
length/MAX_TOKENS -> length, tool_calls -> tool_use, else unknown).
Request encode/decode, streaming, and provider-surface registration
follow in subsequent changes.
Signed-off-by: Federico Kamelhar <federico.kamelhar@oracle.com>
21d6e18 to
1b8a31f
Compare
|
Second CodeRabbit pass triaged:
Suite on the updated tip: 34 OCI codec tests green, clippy zero warnings, full stack rebased. |
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/codec/oci_genai.rs`:
- Around line 313-333: The decode_response_tool_call function currently assigns
an empty string when the OCI response omits id, causing parallel Cohere tool
calls to collide. Replace the unwrap_or_default fallback with a stable, unique
per-call identifier derived from the available tool-call data, while preserving
the provided id unchanged when present.
🪄 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: bb97e075-db42-4f75-be55-471c3f8be155
📒 Files selected for processing (5)
crates/core/src/codec/mod.rscrates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/types/src/codec/request.rscrates/types/src/codec/response.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (17)
**/*.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/types/src/codec/request.rscrates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/types/src/codec/request.rscrates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/types/src/codec/request.rscrates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/types/src/codec/request.rscrates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/types/src/codec/request.rscrates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
{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/types/src/codec/request.rscrates/types/src/codec/response.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/types/src/codec/request.rscrates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/core/src/codec/oci_genai.rs
crates/types/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Runtime helpers must cover marks, scopes, continuations, and isolated scope stacks.
Files:
crates/types/src/codec/request.rscrates/types/src/codec/response.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/types/src/codec/request.rscrates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/types/src/codec/request.rscrates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/types/src/codec/request.rscrates/core/src/codec/mod.rscrates/types/src/codec/response.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/mod.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/unit/codec/oci_genai_tests.rs
🔇 Additional comments (5)
crates/types/src/codec/request.rs (1)
537-549: LGTM!crates/types/src/codec/response.rs (1)
329-339: LGTM!crates/core/src/codec/oci_genai.rs (1)
1-354: Rest of the file (key-convention helpers, GENERIC content decoding, envelope/chat_response split, usage mapping, finish-reason mapping) is correct and the previously-flagged empty-content and error-path issues are now addressed.crates/core/src/codec/mod.rs (1)
19-19: LGTM!crates/core/tests/unit/codec/oci_genai_tests.rs (1)
1-247: LGTM! Both previously-flagged gaps (invalid-content error path, mixed-partsProviderNativecoverage) are now covered.
|
@fede-kamel A few questions:
If Oracle strictly adheres to Cohere, then I'd expect separate CohereV1 and CohereV2 Codecs rather than an Oracle-specific codec.
This seems generally problematic to me. Schema specifications should be concrete and well-defined. If Oracle is extending the Cohere schema to be case/format insensitive, I really need good justification as to why a vendor-specific schema should live in Relay itself. Cohere's API is clearly defined with |
Validated the codec against live OCI Generative AI responses across
model families (Meta, OpenAI, Google, xAI, Cohere) and the three
renderings Oracle tooling emits for the documented camelCase schema.
Changes from that sweep:
- Unwrap the OCI CLI data envelope so captured CLI output decodes like
wire and SDK payloads; document the transport matrix in the module
doc (REST wire/SDKs camelCase, oci.util.to_dict() snake_case, CLI
kebab-case in a data envelope)
- Map the lowercase max_tokens finish reason emitted by Gemini models
to FinishReason::Length
- Map promptTokensDetails.cachedTokens (OpenAI/xAI models) to
cache_read_tokens instead of dropping it
- Synthesize positional call_{index} ids for COHERE tool calls, which
carry no id on the wire, so parallel calls stay distinguishable
- Add unit tests for each live-observed shape and a pipeline
integration test decoding an OCI GENERIC tool-call response through
llm_call_execute
Signed-off-by: Federico Kamelhar <federico.kamelhar@oracle.com>
|
Thanks for the careful review @willkill07 — these are exactly the right questions. Rather than answer from the docs alone, I captured live responses from the service (us-chicago-1, one model from each family: Meta, OpenAI, Google, xAI, Cohere) and checked every claim below against them. 1. Cohere v1 or v2? Neither, and that's the crux: Oracle does not strictly adhere to Cohere. The COHERE format borrows Cohere v1 semantics (single top-level {
"modelId": "cohere.command-r-08-2024",
"modelVersion": "2.0",
"chatResponse": {
"apiFormat": "COHERE",
"text": "...",
"chatHistory": [{"role": "USER", "message": "..."}, {"role": "CHATBOT", "message": "..."}],
"finishReason": "COMPLETE",
"toolCalls": [{"name": "get_weather", "parameters": {"city": "Paris"}}],
"usage": {"promptTokens": 29, "completionTokens": 37, "totalTokens": 66}
}
}A native CohereV1 codec (snake_case, no envelope) would decode none of this. If Relay later adds Cohere's own API as a provider surface, CohereV1/V2 codecs make sense there — but they'd be decoding a different wire format than this one. I also checked the newest Cohere model on OCI ( 2. Streaming Intentional for this layer — streaming is layer 3 of the series ( 3. GENERIC spec It's Oracle's own documented schema:
So routing GENERIC through the OpenAI codec isn't an option; it hard-errors on the tool-call shape and misses the envelope and usage fields. I also verified GENERIC is what every non-Cohere family returns on OCI ( 4. Key conventions Agreed that a case/format-insensitive schema would be a smell, and the module doc described this poorly — I've reworded it in the latest push. To state it precisely: Oracle's schema is concrete, and the REST wire is strictly camelCase, always. The three spellings exist because Oracle's own tooling re-keys the same schema deterministically at different capture points:
I verified all three by running the same call through each path; the test fixtures now mirror those captures. The lookup is not fuzzy matching — the codec derives the kebab/snake spelling mechanically from the documented camelCase key, so there's exactly one accepted rendering per convention and no ambiguity. The snake_case path matters most in practice: Python is Relay's biggest binding, and That said, if you'd rather the built-in codec accept only the wire format and leave CLI/SDK-dict renderings to the caller, I'm happy to trim to camelCase-only — small diff, and I'd rather match the project's conventions than defend a preference. The live sweep also surfaced a few real-world shapes the first cut missed, all fixed with tests in the latest commit: Gemini models emit a lowercase |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/core/src/codec/oci_genai.rs (1)
245-249: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle
COHEREV2as a Cohere response
COHEREV2also carries its generated text inmessage, but this branch sends it to the generic parser, which expectschoicesand drops valid V2 responses. Add an explicitCOHEREV2case here, or reject unsupported formats instead of treating them as generic.🤖 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/codec/oci_genai.rs` around lines 245 - 249, Update the api_format branch in the response decoding flow to route both "COHERE" and "COHEREV2" through decode_cohere_response_body; ensure COHEREV2 no longer reaches decode_generic_response_body, which expects choices.
🤖 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/tests/integration/pipeline_tests.rs`:
- Around line 1442-1515: Make subscriber cleanup in the OCI response codec test
panic-safe by introducing an RAII guard immediately after registering the
subscriber, using its drop behavior to call deregister_subscriber for
"oci_resp_codec_sub". Remove the explicit cleanup dependency at the end of the
test while preserving deregistration on successful completion and during
assertion unwinding.
---
Outside diff comments:
In `@crates/core/src/codec/oci_genai.rs`:
- Around line 245-249: Update the api_format branch in the response decoding
flow to route both "COHERE" and "COHEREV2" through decode_cohere_response_body;
ensure COHEREV2 no longer reaches decode_generic_response_body, which expects
choices.
🪄 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: d86502fa-3656-483c-99da-0672bf8768be
📒 Files selected for processing (3)
crates/core/src/codec/oci_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
**/*.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.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/codec/oci_genai.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/codec/oci_genai.rs
🔇 Additional comments (4)
crates/core/src/codec/oci_genai.rs (2)
13-18: 📐 Maintainability & Code QualityRecord the required core-runtime validation matrix.
Please attach CI evidence for
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,just test-rust,cargo deny check,validate-change, the Rust/Python/Go/Node matrix, anduv run pre-commit run --all-files.As per coding guidelines, changed
crates/corecode requires these Rust checks,validate-change, dependency validation, and the full language matrix.Source: Coding guidelines
219-224: LGTM!Also applies to: 297-371
crates/core/tests/unit/codec/oci_genai_tests.rs (1)
131-206: LGTM!Also applies to: 305-358, 368-369
crates/core/tests/integration/pipeline_tests.rs (1)
33-33: LGTM!
Relay only interfaces with the wire format, so yes. |
Per review, Relay only interfaces with the wire format, so the codec now accepts the documented camelCase schema only. Removes the kebab-case/snake_case key derivation and the CLI data-envelope unwrapping; converting alternate renderings produced by Oracle tooling is the caller's responsibility. Adds a contract test asserting non-wire renderings are not decoded. Signed-off-by: Federico Kamelhar <federico.kamelhar@oracle.com>
|
Done in 7282cdd — the codec now accepts the camelCase wire format only. Dropped the kebab/snake key derivation and the CLI |
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/tests/unit/codec/oci_genai_tests.rs`:
- Around line 106-124: Extend test_non_wire_renderings_are_not_decoded with a
kebab-case CLI-shaped response wrapped in {"data": {...}}, using the documented
non-REST field names. Decode it with OCIGenAIChatCodec and assert model,
message, and finish_reason are all None, matching the existing snake_case case.
🪄 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: 6bd01613-ee5b-40e4-9587-2f9cfce7b63d
📒 Files selected for processing (2)
crates/core/src/codec/oci_genai.rscrates/core/tests/unit/codec/oci_genai_tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
**/*.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/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/unit/codec/oci_genai_tests.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/codec/oci_genai.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/codec/oci_genai.rs
🔇 Additional comments (1)
crates/core/src/codec/oci_genai.rs (1)
168-196: 📐 Maintainability & Code QualityProvide the required core validation evidence.
The summary reports formatting, workspace tests, and Clippy, but not
just test-rust,cargo deny check,validate-change,uv run pre-commit run --all-files, or the required Rust/Python/Go/Node matrix forcrates/corechanges. Attach the corresponding CI or command logs before handoff.As per coding guidelines, “If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js,” and core runtime changes must “satisfycargo deny checkperdeny.toml.”Source: Coding guidelines
Extends the non-wire-rendering contract test with a kebab-case data-enveloped CLI-shaped response, guarding the REST-only boundary against regression on both alternate renderings. Signed-off-by: Federico Kamelhar <federico.kamelhar@oracle.com>
willkill07
left a comment
willkill07
left a comment
There was a problem hiding this comment.
Thanks for splitting this into a focused first step. I found two issues that need addressing before this response codec is ready to merge.
| .unwrap_or("GENERIC") | ||
| .to_uppercase(); | ||
|
|
||
| let (message, tool_calls, finish_reason) = if api_format == "COHERE" { |
There was a problem hiding this comment.
Could we handle COHEREV2 explicitly here? OCI lists it as a supported apiFormat, but this branch sends it through decode_generic_response_body.
V2 responses use message and finishReason rather than choices, so Relay currently records no response text, tool calls, or finish reason—without surfacing an error. Please add a V2 decoder (or return an explicit unsupported-format error) and cover it with a fixture.
If you are introducing OCI support, it seems odd to not also support COHEREV2
There was a problem hiding this comment.
Done in 2ab7e34 — added a dedicated COHEREV2 decoder rather than an error, since the shape is fully specified. Two notes from verifying this:
- The live service doesn't accept the format yet: I probed us-chicago-1 yesterday with
apiFormat: COHEREV2againstcohere.command-a-03-2025andcohere.command-a-reasoning(both v2-messages and v1 shapes) and got HTTP 400 "Please pass in correct format of request" in all cases. It is, however, in the current SDK spec (oci2.182.1 definesCohereChatResponseV2and dispatchesCOHEREV2to it), so the decoder and fixture mirror that published schema exactly: single assistantmessagewith typed content parts, nested-functiontoolCalls({id, type: FUNCTION, function: {name, arguments}}), top-level responseid, and the V2 finish reasonsTOOL_CALL/STOP_SEQUENCE. - TEXT parts flatten like GENERIC; THINKING/IMAGE_URL/DOCUMENT parts are preserved losslessly as provider-native parts.
Covered by test_cohere_v2_chat_result and test_cohere_v2_text_only_flattens, plus the finish-reason mapping cases. I'll re-probe when the service starts accepting the format and adjust if the wire diverges from the spec.
There was a problem hiding this comment.
Update with live-wire evidence: the service accepts apiFormat: COHEREV2 as of today (the same request returned HTTP 400 on 07-28), so I swept every Cohere chat model in the us-chicago-1 catalog with both formats:
| Model | COHERE (v1) | COHEREV2 |
|---|---|---|
cohere.command-a-03-2025 |
✅ accepted | ✅ accepted |
cohere.command-a-vision |
❌ 400 apiFormat COHERE is not supported |
✅ accepted |
cohere.command-latest |
✅ accepted | ❌ 400 not supported |
cohere.command-plus-latest |
✅ accepted | ❌ 400 not supported |
cohere.command-r-08-2024 |
✅ accepted | ❌ 400 not supported |
cohere.command-r-plus-08-2024 |
✅ accepted | ❌ 400 not supported |
cohere.command-a-reasoning, command-r-16k, command-r-plus |
404 (not served on-demand) | 404 |
So the split is generational: the older command/command-r line is V1-only, the current command-a line is V2-first, and command-a-vision is V2-only — without this decoder Relay would record nothing for that model. command-a-03-2025 serves both formats and its wire responses were decoded through the codec for each.
Live V2 responses (text and parallel tool calls) match the CohereChatResponseV2 fixture exactly: nested-function tool calls with provider-supplied ids, JSON-encoded string arguments (parsed like GENERIC), TOOL_CALL finish reason, and message-level toolPlan/citations (now preserved in extra per the other thread). The fixture comment records the live confirmation as of 07ded78.
| api_format: Some(api_format), | ||
| model_version, | ||
| }), | ||
| extra: serde_json::Map::new(), |
There was a problem hiding this comment.
Could we retain the response fields that are not normalized instead of always returning an empty extra map?
This currently discards OCI data such as timeCreated, service-tier/grounding metadata, and future provider fields that are useful for observability.
Please preserve unknown envelope fields and/or retain the raw chatResponse in the OCI-specific response data, consistent with the other response codecs.
There was a problem hiding this comment.
Done in 2ab7e34 — decode now carries all unmodeled fields in extra, matching the flatten-passthrough behavior of the OpenAI/Anthropic response codecs: unknown envelope fields (anything besides modelId/modelVersion/chatResponse) plus chat-response fields outside the per-format modeled set. Concretely that preserves timeCreated, serviceTier, COHERE chatHistory, grounding metadata, and any future provider fields. Covered by test_unmodeled_response_fields_preserved_in_extra, which also asserts modeled keys aren't duplicated into extra.
There was a problem hiding this comment.
Follow-up in 9cae2fb — auditing the preservation claim field-by-field against the OCI schema (ChatChoice, AssistantMessage, CohereAssistantMessageV2) exposed a second layer of the same gap: fields nested inside the decoded choice and assistant message were still dropped. Per the schema that's logprobs, serviceTier, groundingMetadata, usage (choice) and refusal, annotations, reasoningContent (message) for GENERIC, and toolPlan, citations for COHEREV2 — so the grounding metadata you called out actually lives one level deeper than the chat-response pass I'd added. These are now carried in extra, namespaced under "choice" and "message" so their origin stays unambiguous.
Re-validated against live wire responses (us-chicago-1): a GENERIC reply from an OpenAI-family model carries choice-level logprobs and message-level annotations on the wire, both now preserved (previously silently dropped); COHERE v1 behavior is unchanged. Same pass also mapped GENERIC content_filter finish reasons to the normalized ContentFilter, matching the OpenAI codec.
Per review on the OCI codec: - Add a COHEREV2 decoder matching the OCI CohereChatResponseV2 schema: single assistant message with typed content parts (TEXT flattened, THINKING/IMAGE_URL/DOCUMENT preserved as provider-native), nested OpenAI-style function tool calls, response id, and the V2 finish reasons TOOL_CALL and STOP_SEQUENCE. The live service does not accept the format yet (probed us-chicago-1: HTTP 400), so the fixture mirrors the published SDK schema. - Preserve envelope and chat-response fields the normalized shape does not model (timeCreated, serviceTier, chatHistory, grounding metadata, future fields) in extra instead of discarding them, consistent with the other response codecs. Signed-off-by: Federico Kamelhar <federico.kamelhar@oracle.com>
Verified the decoder against the OCI SDK schema (ChatChoice, AssistantMessage, CohereAssistantMessageV2) and closed two gaps: - Unmodeled fields of the decoded GENERIC choice (logprobs, usage, groundingMetadata, serviceTier) and assistant message (refusal, annotations, reasoningContent), plus COHEREV2 message fields (toolPlan, citations), were silently dropped; they are now carried in extra, namespaced under "choice" and "message". - content_filter finish reasons from OpenAI-style GENERIC models now map to FinishReason::ContentFilter, matching the OpenAI codec. Signed-off-by: Federico Kamelhar <federico.kamelhar@oracle.com>
The merge from main brought in the unified OTel exporter (NVIDIA#556), whose exhaustive match on ApiSpecificRequest did not cover the OCIGenAI variant this branch introduces, breaking the workspace build. Map it to "oci.genai", following the dotted cloud-provider convention used by aws.bedrock and gcp.gemini. Signed-off-by: Federico Kamelhar <federico.kamelhar@oracle.com>
github-actions
Bot
added
size:XL
|
Status after the branch update merging Validation on the current tip: full workspace suite green (3,485 tests), clippy zero warnings, |
The service now accepts apiFormat COHEREV2 (it rejected it with HTTP 400 the day before); live text and parallel tool-call responses match the spec-mirrored fixture exactly, so the fixture comment no longer claims the format is spec-only. Signed-off-by: Federico Kamelhar <federico.kamelhar@oracle.com>
fede-kamel
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/core/src/codec/oci_genai.rs (1)
148-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-string
TEXT.textvalues.
{"type":"TEXT","text":1}bypasses text flattening, then becomesText("")here viaunwrap_or_default(). ReturnInvalidArgumentinstead of silently altering provider content, and add a regression fixture.Proposed fix
Some("TEXT") => Ok(ContentPart::Text { - text: obj - .get("text") - .and_then(Json::as_str) - .unwrap_or_default() - .to_string(), + text: match obj.get("text") { + None | Some(Json::Null) => String::new(), + Some(Json::String(text)) => text.clone(), + Some(_) => { + return Err(FlowError::InvalidArgument( + "OCI GenAI TEXT content part text must be a string or null".into(), + )); + } + },🤖 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/codec/oci_genai.rs` around lines 148 - 159, Update the TEXT branch of the OCI content decoder to distinguish a missing text field from a present non-string value: reject non-string TEXT.text values with InvalidArgument instead of defaulting them to an empty string. Preserve the existing handling for valid strings and absent fields as intended, and add a regression fixture covering {"type":"TEXT","text":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.
Outside diff comments:
In `@crates/core/src/codec/oci_genai.rs`:
- Around line 148-159: Update the TEXT branch of the OCI content decoder to
distinguish a missing text field from a present non-string value: reject
non-string TEXT.text values with InvalidArgument instead of defaulting them to
an empty string. Preserve the existing handling for valid strings and absent
fields as intended, and add a regression fixture covering
{"type":"TEXT","text":1}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: dcdc699c-5639-4eb9-bffb-94647cbc0b4f
📒 Files selected for processing (4)
crates/core/src/codec/oci_genai.rscrates/core/src/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
**/*.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/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/src/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/observability/otel_genai.rscrates/core/src/codec/oci_genai.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/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/core/src/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/observability/otel_genai.rscrates/core/tests/integration/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.rscrates/core/src/codec/oci_genai.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/pipeline_tests.rscrates/core/tests/unit/codec/oci_genai_tests.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/codec/oci_genai.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/observability/otel_genai.rscrates/core/src/codec/oci_genai.rs
🔇 Additional comments (8)
crates/core/src/codec/oci_genai.rs (2)
11-64: LGTM!Also applies to: 126-139, 189-257, 269-445
176-260: 📐 Maintainability & Code QualityAttach the required core validation evidence.
The supplied summary reports workspace Rust checks, but not the required full core matrix. Please provide results for
just test-rust,cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,cargo deny check,uv run pre-commit run --all-files, and the configured Python, Go, and Node test targets.
crates/core/src/codec/oci_genai.rs#L176-L260: validate OCI response decoding through the required core matrix.crates/core/src/observability/otel_genai.rs#L390-L401: validate the OCI provider projection through the same matrix.Source: Coding guidelines
crates/core/src/observability/otel_genai.rs (1)
1-388: LGTM!Also applies to: 404-523
crates/core/tests/unit/codec/oci_genai_tests.rs (2)
106-146: LGTM!
300-507: LGTM!crates/core/tests/integration/pipeline_tests.rs (3)
33-33: LGTM!Also applies to: 1476-1517
1536-1536: LGTM!Also applies to: 1736-1741, 2004-2004
1451-1475: 🗄️ Data Integrity & IntegrationNo issue: GENERIC
toolCallsshape matches the decoder
OCIGenAIChatCodecaccepts directid/name/argumentsentries fortoolCalls, and also falls back to a nestedfunctionobject for CohereV2-style payloads.> Likely an incorrect or invalid review comment.
2 participants
Overview
First PR of a four-part stacked series adding Oracle Cloud Infrastructure (OCI) Generative AI as a built-in provider in the Rust core — the rework requested in #549 review, split into foundation-ordered units of ~≤1k lines for reviewability (per the plan posted on #552 before closing it).
Series map (each stacked on the previous, opened as its predecessor merges):
ApiSpecificvariants + response codec (~975 lines)Details
What:
ApiSpecificRequest::OCIGenAI { compartment_id, serving_mode, api_format }andApiSpecificResponse::OCIGenAI { api_format, model_version }(serde tag"oci_genai") incrates/types, pluscrates/core/src/codec/oci_genai.rsintroducingOCIGenAIChatCodecwith itsLlmResponseCodecimplementation: decodes OCIChatResultpayloads across all three documentedapiFormats —GENERIC(choices-based; Meta Llama, Google, xAI, OpenAI, and imported open-weights models such as NVIDIA Nemotron on dedicated AI clusters),COHERE(text-based), andCOHEREV2(single assistantmessagewith typed content parts and nested-function tool calls, per the OCICohereChatResponseV2schema) — normalizing model, response id, message content, tool calls, finish reasons, and usage counters (includingpromptTokensDetails.cachedTokens). The codec accepts the REST wire format only (camelCase, as documented); alternate renderings produced by Oracle tooling are the caller's responsibility. Envelope and chat-response fields outside the normalized shape (timeCreated,serviceTier,chatHistory, grounding metadata, future provider fields) are preserved inextrarather than discarded, consistent with the other response codecs.Why: OCI GenAI responses are currently opaque to Relay observability. This layer alone enables normalized
LLMEndannotations (model, finish reason, token usage) for OCI-backed calls when the codec is supplied explicitly; subsequent PRs add request editing, automatic detection, and binding exposure.How: Follows the built-in provider pattern (
anthropic.rsas template): unit-struct codec, in-module tests,FinishReasonmapping for the three formats' vocabularies (stop/length/max_tokens/tool_calls;COMPLETE/MAX_TOKENS;TOOL_CALL/STOP_SEQUENCE). COHERE tool calls carry noidon the wire, so parallel calls receive positionalcall_{index}fallbacks; provider-supplied ids pass through unchanged.Testing: 13 unit tests plus a pipeline integration test decoding a GENERIC tool-call response through
llm_call_execute. GENERIC and COHERE fixtures are taken from live OCI wire captures (simple, tool-call, mixed-content, and tool-call-only shapes, plus a model-family sweep across Meta, OpenAI, Google, xAI, and Cohere confirming no additional response formats); the COHEREV2 fixture mirrors the publishedCohereChatResponseV2SDK schema, as the live service does not yet accept that format (probed: HTTP 400). Additional coverage: wire-format-only contract (snake_case and kebab-case/data-envelope renderings decode to nothing), invalid-content error paths, ProviderNative part preservation, unmodeled-field preservation inextra, finish-reason and cached-token usage mapping, and non-dict fallback. Full workspace suite green (3,377 tests),cargo clippy --workspace --all-targetszero warnings,cargo fmtclean,missing_docssatisfied. The complete series content has additionally been validated end-to-end: the encoded payload of the full codec was posted verbatim to the signed OCI REST endpoint and answered by a dedicated AI cluster serving an imported NVIDIA Nemotron 3 model.Breaking changes: None — additive enum variants and a new module only.
Where should the reviewer start?
crates/types/src/codec/{request,response}.rsfor the variant shapes, thendecode_responseand the fixtures incrates/core/tests/unit/codec/oci_genai_tests.rs.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Tests