fix: return middleware-translated buffered responses#496
Conversation
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
WalkthroughThe buffered gateway path reconciles upstream JSON with runtime output before constructing responses. The process-boundary e2e test adds a ChangesGateway response flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/cli/tests/switchyard_process_e2e.rs`:
- Around line 315-322: Strengthen the assertions for providers[1] in the process
boundary response test: retain the existing messages-array and absent-input
checks, then verify the translated message uses the expected user role and
contains the original “process boundary response translation” text in the
protocol’s actual message representation.
🪄 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: 47f00c97-0acb-44ca-953a-4c856e2609ed
📒 Files selected for processing (2)
crates/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Check / Run
🧰 Additional context used
📓 Path-based instructions (10)
**/*.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/tests/switchyard_process_e2e.rs
🔇 Additional comments (4)
crates/cli/src/gateway/mod.rs (3)
84-84: LGTM!Also applies to: 243-243
222-226: 🗄️ Data Integrity & IntegrationVerify body-dependent headers when replacing upstream bytes.
When Lines 223-224 replace the upstream body,
build_responseincrates/cli/src/gateway/response.rscopies every supplied header. Confirmresponse_headersfilters or recomputescontent-lengthandcontent-encoding; otherwise clients may receive truncated or incorrectly decoded responses.
217-243: 📐 Maintainability & Code QualityRun the required Rust checks before handoff.
The supplied context does not include results for
cargo fmt --all,just test-rust,cargo clippy --workspace --all-targets -- -D warnings, oruv run pre-commit run --all-files.As per coding guidelines, Rust changes must pass formatting, tests, clippy, and pre-commit validation.
Source: Coding guidelines
crates/cli/tests/switchyard_process_e2e.rs (1)
46-46: LGTM!Also applies to: 239-239, 257-290
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
…ay-510-nemo-relay060-rc2-switchyard-v1responses-returns-chat Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com> # Conflicts: # crates/cli/tests/switchyard_process_e2e.rs
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 (2)
crates/cli/src/gateway/mod.rs (1)
217-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve unchanged non-JSON upstream bodies.
When upstream parsing fails, this path serializes
response_json. However,build_buffered_funcrepresents unchanged non-JSON responses as{"body_bytes": bytes.len()}, so an unchanged text, empty, or binary response is replaced by synthetic JSON instead of the original buffered bytes. Carry an explicit parse/unchanged marker so raw bytes are retained unless middleware actually changes the response.Also applies to: 325-335
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cli/src/gateway/mod.rs` around lines 217 - 229, Update the response-body handling in build_buffered_func, including the corresponding path around the additional reported range, to track whether upstream JSON parsing succeeded and whether middleware changed the response. Preserve and return the original buffered bytes for unchanged, non-JSON, empty, or binary bodies; only serialize response_json when middleware actually modifies the upstream response.crates/cli/tests/switchyard_process_e2e.rs (1)
286-288: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the
/v1/responsespayload content, not justobject.
object == "response"still passes if the translated body drops or alters the returned content. Add an assertion on the actual response fields so this test covers response translation end to end.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cli/tests/switchyard_process_e2e.rs` around lines 286 - 288, Expand the assertions in the `/v1/responses` test after deserializing `translated` to validate the returned content fields, not only `translated["object"]`. Reuse the expected response values established by the test and assert the translated payload preserves them end to end.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/cli/src/gateway/mod.rs`:
- Around line 217-229: Update the response-body handling in build_buffered_func,
including the corresponding path around the additional reported range, to track
whether upstream JSON parsing succeeded and whether middleware changed the
response. Preserve and return the original buffered bytes for unchanged,
non-JSON, empty, or binary bodies; only serialize response_json when middleware
actually modifies the upstream response.
In `@crates/cli/tests/switchyard_process_e2e.rs`:
- Around line 286-288: Expand the assertions in the `/v1/responses` test after
deserializing `translated` to validate the returned content fields, not only
`translated["object"]`. Reuse the expected response values established by the
test and assert the translated payload preserves them end to end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1b09ea38-a969-4a5c-ab70-5a22ccea7478
📒 Files selected for processing (2)
crates/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
**/*.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/src/gateway/mod.rscrates/cli/tests/switchyard_process_e2e.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/cli/tests/switchyard_process_e2e.rs
🔇 Additional comments (2)
crates/cli/src/gateway/mod.rs (1)
84-84: LGTM!Also applies to: 243-243
crates/cli/tests/switchyard_process_e2e.rs (1)
46-46: LGTM!Also applies to: 90-102, 251-251, 269-285, 290-300, 301-307, 332-346, 347-359
|
/merge |
Overview
Fix the buffered CLI gateway so clients receive the final JSON returned by LLM middleware, including Switchyard response translations, instead of always receiving the raw upstream bytes.
Details
/v1/responsesrequest, assert that the provider receives a translated Chat Completions request, and assert that the client receives an OpenAI Responses payload.Validation:
cargo test -p nemo-relay-cli --features switchyard --test switchyard_process_e2e -- --nocapturecargo test -p nemo-relay-cli --features switchyard -- --test-threads=1cargo fmt --allcargo clippy --workspace --all-targets -- -D warningscargo clippy -p nemo-relay-cli --all-targets --features switchyard -- -D warningsjust test-rustran the Rust matrix; one unrelated CLI test encountered shared temporary plugin configuration from a concurrent test and passed when rerun independently.uv run pre-commit run --all-files; all applicable checks passed except the baseline Rust attribution generator, which rewrites the unrelated checked-inmd-5license entry. A final all-files run with only that generator skipped passed.Where should the reviewer start?
Start with
crates/cli/src/gateway/mod.rsinrun_managed_buffered, then review the process-level regression incrates/cli/tests/switchyard_process_e2e.rs.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
/v1/responsesflows to better align requests and responses.