fix(gateway): decode compressed request bodies - #452
Conversation
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (8)**/*.{md,rst,html,txt}📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
Files:
**/*.{md,rst,html}📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
Files:
**/*.{md,rst,txt}📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
Files:
**/*.{md,rst}📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
Files:
**/*.md📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-technical-docs.md)
Files:
**/*.{md,mdx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{md,markdown,mdx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
🪛 markdownlint-cli2 (0.23.0)ATTRIBUTIONS-Rust.md[warning] 24924-24924: Headings should be surrounded by blank lines (MD022, blanks-around-headings) [warning] 24924-24924: Headings should be surrounded by blank lines (MD022, blanks-around-headings) [warning] 24925-24925: Headings should be surrounded by blank lines (MD022, blanks-around-headings) [warning] 24925-24925: Headings should be surrounded by blank lines (MD022, blanks-around-headings) [warning] 25130-25130: Headings should be surrounded by blank lines (MD022, blanks-around-headings) [warning] 25131-25131: Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) [warning] 25131-25131: Fenced code blocks should have a language specified (MD040, fenced-code-language) 🔇 Additional comments (1)
WalkthroughGateway request preparation now supports bounded zstd observability decoding while preserving upstream passthrough behavior. Re-encoded outbound bodies remove stale ChangesGateway encoding handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant IncomingRequest
participant prepare_gateway_request
participant request_body_for_observability
participant decode_zstd
IncomingRequest->>prepare_gateway_request: buffered body and Content-Encoding
prepare_gateway_request->>request_body_for_observability: body bytes and encoding headers
request_body_for_observability->>decode_zstd: zstd payload within size limit
decode_zstd-->>request_body_for_observability: decoded bytes or failure
request_body_for_observability-->>prepare_gateway_request: observable body or None
prepare_gateway_request-->>IncomingRequest: request_json and original passthrough body
🚥 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: 3
🤖 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/src/gateway/request.rs`:
- Around line 90-118: Update request_body_for_observability to accept a
lifetime-annotated body slice and return Cow<[u8]> (or the equivalent
borrowed/owned type), borrowing the original body when Content-Encoding is
absent or identity-only and retaining owned decoded data for zstd. Update the
caller’s handling as needed while preserving the existing serde_json::from_slice
behavior and size limits.
- Around line 120-128: Update decode_zstd to cap the zstd decoder’s internal
window using Decoder::set_parameter before reading any decompressed data,
deriving the cap from max_decoded_bytes and handling parameter-setting failure.
Retain the existing take(limit) output bound and decoded-length validation.
In `@crates/cli/tests/coverage/shared/gateway_tests.rs`:
- Around line 106-203: Add a test near
request_observability_decode_is_bounded_and_encoding_aware that builds a body
with multiple Content-Encoding layers and verifies prepare_gateway_request
decodes them in reverse application order, such as applying zstd twice or using
gzip followed by zstd. Assert the resulting request_json matches the original
payload and preserve the existing single-encoding and malformed-input coverage.
🪄 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: 2e0dff3e-0cfa-48c0-8f0c-cb86e411f7f1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/cli/Cargo.tomlcrates/cli/src/gateway/mod.rscrates/cli/src/gateway/request.rscrates/cli/tests/coverage/shared/gateway_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (11)
**/Cargo.toml
📄 CodeRabbit inference engine (.agents/skills/prepare-code-freeze/SKILL.md)
Confirm or infer the target release version from
upstream/main:Cargo.toml. Derive the release branch asrelease/<major>.<minor>.Keep Rust package names and workspace metadata in
Cargo.tomlinternally consistent across the project.
Files:
crates/cli/Cargo.toml
**/*.toml
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all TOML files using the
#comment form.
Files:
crates/cli/Cargo.toml
**/*
📄 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/Cargo.tomlcrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.rscrates/cli/src/gateway/mod.rs
**/*.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/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.rscrates/cli/src/gateway/mod.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.rscrates/cli/src/gateway/mod.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/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.rscrates/cli/src/gateway/mod.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/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.rscrates/cli/src/gateway/mod.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/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.rscrates/cli/src/gateway/mod.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/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.rscrates/cli/src/gateway/mod.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/coverage/shared/gateway_tests.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/request.rscrates/cli/src/gateway/mod.rs
🔇 Additional comments (5)
crates/cli/Cargo.toml (1)
69-69: 📐 Maintainability & Code QualityDependency addition looks fine.
zstd = "0.13"is a valid, current version range (0.13.3 latest patch) for this well-established binding crate.Please confirm
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, andjust test-rustwere run for this Rust change, as required for any Rust-touching PR. As per coding guidelines, "Any Rust change must runjust test-rust... Runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit."Source: Coding guidelines
crates/cli/src/gateway/request.rs (1)
37-56: LGTM!crates/cli/tests/coverage/shared/gateway_tests.rs (2)
136-153: 🎯 Functional Correctness | ⚡ Quick winTest couples raw-ingestion limit and decoded-size limit via the same config field.
max_passthrough_body_bytes: 32bounds both the compressed request body accepted byaxum::body::to_bytesand the decompressed output size checked indecode_zstd. The assertion thatprepared.request_json.is_null()here implicitly relies on the zstd-compressed form of 256 repeated bytes staying under 32 bytes so it isn't rejected earlier by the raw ingestion limit. This holds today but is an unstated dependency on the compression library's output size for this exact fixture.Consider asserting/documenting the compressed size explicitly, or using distinct config values for the raw-body cap vs. the decode-size-limit scenario, to decouple the test from compression-ratio assumptions.
183-202: LGTM!Also applies to: 397-415
crates/cli/src/gateway/mod.rs (1)
20-22: LGTM!Also applies to: 774-782, 821-823
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
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 `@ATTRIBUTIONS-Rust.md`:
- Around line 22937-22941: Update each specified attribution block, including
the jobserver section and the blocks at the referenced locations, to add blank
lines around headings and fenced code blocks. Add an appropriate language
identifier to every license fence, preserving the existing attribution content.
- Around line 22939-22940: Update the Rust attribution generation and licensing
configuration so md-5, pkg-config, zstd-safe, zstd-sys, and jobserver 0.1.35
retain both MIT and Apache-2.0 SPDX notices. Prefer preserving the dual-license
notices in ATTRIBUTIONS-Rust.md; if Apache-only output is intentional,
explicitly configure that policy in
scripts/licensing/attributions_lockfile_md.py and about.toml.
🪄 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: 464e59f9-546f-4dc9-9d4a-959c1dba021d
📒 Files selected for processing (1)
ATTRIBUTIONS-Rust.md
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{md,rst,html,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
**/*.{md,rst,html,txt}: Always spellNVIDIAin all caps. Do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun because the name starts with an 'en' sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company.
Use trademark symbols with product names only when the document type or legal guidance requires them.
Verify official capitalization, spacing, and hyphenation for product names.
Precede NVIDIA product names withNVIDIAon first mention when it is natural and accurate.
Do not rewrite product names for grammar or title-case rules.
Preserve third-party product names according to the owner's spelling.
Include the company name and full model qualifier on first use when it helps identify the model.
Preserve the official capitalization and punctuation of model names.
Use shorter family names only after the full name is established.
Spell out a term on first use and put the acronym in parentheses unless the acronym is widely understood by the intended audience.
Use the acronym on later mentions after it has been defined.
For long documents, reintroduce the full term if readers might lose context.
Form plurals of acronyms withs, not an apostrophe, such asGPUs.
In headings, common acronyms can remain abbreviated. Spell out the term in the first or second sentence of the body.
Common terms such asCPU,GPU,PC,API, andUIusually do not need to be spelled out for developer audiences.
Files:
ATTRIBUTIONS-Rust.md
**/*.{md,rst,html}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
Link the first mention of a product name when the destination helps the reader.
Files:
ATTRIBUTIONS-Rust.md
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
Spell
NVIDIAin all caps. Do not useNvidia,nvidia, orNV.
Files:
ATTRIBUTIONS-Rust.md
**/*.{md,rst}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
**/*.{md,rst}: Format commands, code elements, expressions, package names, file names, and paths as inline code.
Use descriptive link text. Avoid raw URLs and weak anchors such as "here" or "read more."
Use title case consistently for technical documentation headings.
Introduce code blocks, lists, tables, and images with complete sentences.
Write procedures as imperative steps. Keep steps parallel and split long procedures into smaller tasks.
Prefer active voice, present tense, short sentences, contractions, and plain English.
Usecanfor possibility and reservemayfor permission.
Useafterfor temporal relationships instead ofonce.
Preferrefer tooverseewhen the wording points readers to another resource.
Avoid culture-specific idioms, unnecessary Latinisms, jokes, and marketing exaggeration in technical docs.
Spell out months in body text, avoid ordinal dates, and use clear time zones.
Spell out whole numbers from zero through nine unless they are technical values, parameters, versions, or UI values.
Use numerals for 10 or greater and include commas in thousands.
Do not add trademark symbols to learning-oriented docs unless the source, platform, or legal guidance explicitly requires them.
Files:
ATTRIBUTIONS-Rust.md
**/*.md
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-technical-docs.md)
**/*.md: Use title case consistently in technical documentation headings
Avoid quotation marks, ampersands, and exclamation marks in headings
Keep product, event, research, and whitepaper names in their official title case
Use title case for table headers
Do not force social-media sentence case into technical docs
Format code elements, commands, parameters, package names, and expressions in monospace
Format directories, file names, and paths in monospace using backticks
Use angle brackets inside monospace for variables inside paths, such as/home/<username>/.login
Format error messages and strings in quotation marks, keeping literal code strings in code formatting when clearer
Format UI buttons, menus, fields, and labels in bold
Use angle brackets between UI labels for menu paths, such as File > Save As
Use italics for new terms on first use, sparingly and only when introducing the term
Use italics for publication titles
Format keyboard shortcuts in plain text, such as Press Ctrl+Alt+Delete
Use owner/repo link text for GitHub repositories, preferring[NVIDIA/NeMo](link)over prose references like 'the GitHub repo'
Introduce every code block with a complete sentence
Do not make a code block complete the grammar of the previous sentence
Do not continue a sentence after a code block
Use syntax highlighting when the format supports it for code blocks
Avoid the word 'snippet' unless the surrounding docs already use it as a term of art
Keep inline method, function, and class references consistent with nearby docs, omitting empty parentheses for prose readability when no call is shown
Use descriptive anchor text that matches the destination title when possible for links
Avoid raw URLs in running text
Avoid generic anchor text such as 'here,' 'this page,' and 'read more'
Include acronyms in link text when a linked term includes an acronym
Do not link long sentences or multiple sentences
Avoid links that pull readers away from a procedure unless the link is a p...
Files:
ATTRIBUTIONS-Rust.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Update
README.md,fern/, package READMEs, and binding-support notes when public behavior, package names, examples, or supported bindings change.
**/*.{md,mdx}: Prefer the documented public API, not internal shortcuts
Keep package names, repo references, and build commands current
Keep release-process and release-notes guidance in repo-maintainer docs such asRELEASING.md, not as user-facing docs pages orCHANGELOG.md
Keep stable user-facing wrappers atscripts/root in docs and examples; only point at namespaced helper paths when documenting internal maintenance work
When detailed dynamic plugin guides exist, keep Rust native plugin examples, Python worker plugin examples, andgrpc-v1protocol details on separate pagesIf links in documentation change, run
just docs-linkcheck.
Files:
ATTRIBUTIONS-Rust.md
**/*.{md,markdown,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Markdown/MDX documentation files using the HTML comment block form.
Files:
ATTRIBUTIONS-Rust.md
**/*
📄 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:
ATTRIBUTIONS-Rust.md
🪛 markdownlint-cli2 (0.23.0)
ATTRIBUTIONS-Rust.md
[warning] 22937-22937: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 22940-22940: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 22940-22940: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 22941-22941: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 22941-22941: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 24924-24924: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 24924-24924: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 30118-30118: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 30121-30121: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 30121-30121: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 30122-30122: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 30122-30122: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 60338-60338: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 60341-60341: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 60341-60341: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 60342-60342: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 60342-60342: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 60354-60354: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 60357-60357: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 60357-60357: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 60358-60358: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 60358-60358: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 60435-60435: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 60438-60438: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 60438-60438: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 60439-60439: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 60439-60439: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
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/cli/tests/coverage/shared/gateway_tests.rs (1)
179-194: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse valid JSON for the oversized-body fixture.
The repeated
xpayload is invalid JSON, sorequest_json.is_null()still passes if the decoded-size limit is removed. Make it a valid JSON body larger than 32 bytes so this test actually detects an unbounded decode.Proposed fix
- let oversized = vec![b'x'; 256]; + let oversized = format!(r#"{{"model":"{}"}}"#, "x".repeat(256)).into_bytes();As per path instructions, “Tests should cover the behavior promised by the changed API surface, including error paths.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cli/tests/coverage/shared/gateway_tests.rs` around lines 179 - 194, Update the oversized-body fixture in the test around prepare_gateway_request to encode valid JSON whose decompressed size exceeds the 32-byte max_passthrough_body_bytes limit, then retain the existing null assertion. Ensure the assertion fails if the decoded-size limit is removed rather than merely reflecting invalid JSON parsing.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/tests/coverage/shared/gateway_tests.rs`:
- Around line 179-194: Update the oversized-body fixture in the test around
prepare_gateway_request to encode valid JSON whose decompressed size exceeds the
32-byte max_passthrough_body_bytes limit, then retain the existing null
assertion. Ensure the assertion fails if the decoded-size limit is removed
rather than merely reflecting invalid JSON parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 2119fec1-e363-43ae-bfff-5d39f91a3881
📒 Files selected for processing (2)
crates/cli/src/gateway/request.rscrates/cli/tests/coverage/shared/gateway_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (9)
**/*.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/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.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/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.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/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.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/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.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/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/request.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/tests/coverage/shared/gateway_tests.rscrates/cli/src/gateway/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/cli/tests/coverage/shared/gateway_tests.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/request.rs
🔇 Additional comments (3)
crates/cli/src/gateway/request.rs (2)
51-57: LGTM!Also applies to: 91-140
6-6: 📐 Maintainability & Code QualityProvide the required Rust validation results.
Please attach successful runs of
cargo fmt --all,just test-rust,cargo clippy --workspace --all-targets -- -D warnings, and finaluv run pre-commit run --all-files. Confirm whether this gateway runtime change also requirescargo deny check.As per coding guidelines, “Any Rust change must run
just test-rust”, “runcargo fmt --all”, “runcargo clippy --workspace --all-targets -- -D warnings”, and “Before review or handoff, runuv run pre-commit run --all-files.”Source: Coding guidelines
crates/cli/tests/coverage/shared/gateway_tests.rs (1)
136-175: LGTM!
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
Salonijain27
left a comment
There was a problem hiding this comment.
Approved from a dependency point of view
|
/merge |
1 similar comment
|
/merge |
#### Overview Replace the legacy `nvidia.fabric.codex.cli` subprocess adapter with the official Codex Python SDK and the `nvidia.fabric.codex` adapter. Preserve one-shot and multi-turn execution, ChatGPT and API-key authentication, normalized results, and optional NeMo Relay observability while removing Fabric's CLI-output parsing and process-control implementation. This revision also makes semantic Relay output an acceptance criterion. A Fabric-managed Codex run must produce structured LLM model metadata, token usage, and an ATIF agent response comparable to a direct `nemo-relay codex` run. #### Details - Drive Codex through typed SDK thread, turn, interruption, and shutdown APIs. Persist one Codex thread ID per Fabric runtime and reject mismatched resume state. - Keep the Codex SDK as the execution driver. `harness.settings.codex_bin` is an explicit SDK binary override for compatibility testing; it does not restore the removed Fabric CLI adapter. - Keep Relay as an optional supervised gateway and hook forwarder. Fabric retains Codex's built-in `openai` provider and supplies the gateway through request-scoped `openai_base_url`, preserving both cached ChatGPT login and `OPENAI_API_KEY` authentication. - Define the shared Relay CLI contract centrally (`>=0.6.0,<0.7.0`), select exact per-agent hook sets, and disable Relay's nested Codex delegation hook so Fabric owns the single SDK execution lifecycle. - Require semantic Relay artifacts in live Claude and Codex tests: a structured LLM request/model, total token usage, and the expected ATIF agent response. - Treat request-content decoding as a Relay transport concern. Compressed Codex request bodies require [NeMo Relay PR #452](NVIDIA/NeMo-Relay#452); Fabric intentionally has no compression-specific configuration or decoding path. - Preserve Claude SDK parity discovered during cross-adapter live validation: inherit the Unix `USER` selector for cached login, retain typed SDK error results, and update the pinned Claude Agent SDK to 0.2.120. - Document Codex and Claude authentication modes, SDK/runtime pinning, the Relay compatibility range, native Codex OTEL versus Relay OpenInference output, and the current Relay decoding dependency. #### Validation - `just --set no_uv true test-python` after syncing the CI extras: `312 passed, 11 skipped` - Focused common/Claude/Codex/example suite: `95 passed, 5 skipped` - `cargo check -p fabric-python --locked` - `pre-commit run --all-files --show-diff-on-failure` - Docs generation and `fern check`: `0 errors` (one pre-existing warning is not printed by default) - Live Codex SDK plus patched Relay, full one-shot semantic assertion and two-turn resume: - SDK-pinned Codex runtime 0.137.0a4: passed - Explicit latest Codex 0.144.5: passed - Live Claude direct and Claude plus Relay with cached managed login: passed - Mock Claude plus released Relay 0.6.0: passed - Relay decoding dependency: 54 focused gateway tests, Clippy with `-D warnings`, `cargo deny check bans licenses sources`, and the Rust attribution hook passed in NeMo Relay PR #452 #### Where should the reviewer start? Start with `adapters/codex/src/nemo_fabric_adapters/codex/adapter.py`, then `adapters/common/src/nemo_fabric_adapters/common/relay_gateway.py` and `tests/e2e/test_codex.py`. The central design boundary is that the SDK owns Codex execution, Fabric owns runtime/session orchestration, and Relay owns gateway transport decoding plus semantic observability. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to [FABRIC-50](https://linear.app/nvidia/issue/FABRIC-50/replace-codex-cli-with-codex-sdk) - Relates to [NeMo Relay PR #452](NVIDIA/NeMo-Relay#452) - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. Authors: - Ajay Thorve (https://github.com/AjayThorve) Approvers: - Anuradha Karuppiah (https://github.com/AnuradhaKaruppiah) URL: #66
Overview
Decode supported
Content-Encodingvalues before building the gateway's managed LLM request. This restores semantic request attributes for clients such as the Codex app server, which sends Responses API payloads withContent-Encoding: zstd.Details
zstdandidentityencodings in reverse application order for Relay's managed request representation without allocating for unencoded or identity-only bodies.max_passthrough_body_bytesto avoid decompression amplification and oversized decoder allocations.Content-Encodingonly when the managed pipeline reserializes the decoded JSON body as identity bytes.Without this, the request reached the provider successfully but Relay recorded
content: null; OpenInference then emitted opaquebody_bytesand omitted model and token semantics. The fix was also validated through the publishedopenai-codexSDK app-server path and a NeMo Fabric Codex SDK + Relay end-to-end run, where Phoenix received populated chain, LLM, and tool spans.Validation:
cargo test -p nemo-relay-cli gateway::tests -- --nocapture(54 passed)cargo clippy -p nemo-relay-cli --all-targets -- -D warningscargo deny check bans licenses sourcesuvx pre-commit run attributions-rust --all-filesNo public API or configuration changes.
Where should the reviewer start?
Start with
crates/cli/src/gateway/request.rs. The key invariant is that decoding creates a bounded managed representation whilePreparedGatewayRequestretains the original transport body. Then revieweffective_dispatch_requestincrates/cli/src/gateway/mod.rs, which removes the stale encoding header only after JSON reserialization succeeds.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
zstd-compressed request bodies containing valid JSON, with decoding that’s encoding-aware, bounded, and preservesContent-Encodingwhen applicable.Content-Encodingheaders from being sent after gateway request bodies are rewritten/re-encoded.zstddecoding andContent-Encoding/payload edge cases.zstdlicense details.