Skip to content

fix(llm): reject unsupported RL fields for responses - #11253

Merged
jthomson04 merged 1 commit into
mainfrom
jthomson04/dyn-3392-reject-responses-rl-fields
Jul 6, 2026
Merged

fix(llm): reject unsupported RL fields for responses#11253
jthomson04 merged 1 commit into
mainfrom
jthomson04/dyn-3392-reject-responses-rl-fields

Conversation

@jthomson04

@jthomson04 jthomson04 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • reject completion_token_ids and prompt_logprobs when requested through nvext.extra_fields on /v1/responses
  • return a field-specific 501 response before engine dispatch for both streaming and non-streaming requests
  • leave Chat Completions, Completions, and supported Responses nvext fields unchanged

Root cause

Responses requests are converted to the Chat Completions protocol internally. The non-streaming conversion copied the resulting chat response nvext verbatim, while the Responses streaming converter did not expose those fields. This unintentionally made the RL fields available only on the non-streaming Responses path.

Linear: https://linear.app/nvidia/issue/DYN-3392

Validation

  • cargo fmt --all -- --check
  • cargo test -p dynamo-llm test_validate_unsupported_fields --lib (8 passed)
  • cargo test -p dynamo-llm protocols::openai --lib (207 passed)
  • cargo clippy -p dynamo-llm --lib -- -D warnings

Summary by CodeRabbit

  • Bug Fixes
    • Responses API requests now return a clear 501 Not Implemented error when unsupported nvext.extra_fields values are included.
    • Requests using completion_token_ids or prompt_logprobs are now consistently rejected, while supported nvext fields continue to work as expected.
    • Validation behavior is now consistent across both streaming and non-streaming requests.

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
@github-actions github-actions Bot added fix frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` labels Jul 6, 2026
@datadog-official

datadog-official Bot commented Jul 6, 2026

Copy link
Copy Markdown

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 43.14% (-1.27%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 1aefc1a | Docs | Give us feedback!

@biswapanda

Copy link
Copy Markdown
Contributor

changes lgtm

@jthomson04
jthomson04 marked this pull request as ready for review July 6, 2026 19:18
@jthomson04
jthomson04 requested a review from a team July 6, 2026 19:18

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8796c53d-6fd8-4a69-8b0d-71fe463e401c

📥 Commits

Reviewing files that changed from the base of the PR and between b032d7a and 1aefc1a.

📒 Files selected for processing (1)
  • lib/llm/src/http/service/openai.rs

Walkthrough

Adds a validation guard in validate_response_unsupported_fields that rejects Responses API requests containing RL-only nvext.extra_fields values (completion_token_ids or prompt_logprobs), returning a 501 error. Expands unit tests to cover this rejection across streaming modes and mixed/supported field scenarios.

Changes

Responses API nvext Validation

Layer / File(s) Summary
Validation guard and test coverage
lib/llm/src/http/service/openai.rs
validate_response_unsupported_fields now rejects completion_token_ids and prompt_logprobs in nvext.extra_fields with a 501 error; new/updated tests verify rejection across stream true/false, mixed nvext fields, and acceptance of supported fields.

Estimated code review effort: 2 (Simple) | ~10 minutes

Related Issues: None mentioned in the provided context.

Related PRs: None mentioned in the provided context.

Suggested labels: area/http-service, testing

Suggested reviewers: None specified in the provided context.

🐰 A field slips in where it shouldn't be,
completion_token_ids, sneaky as can be—
The Responses gate now stands guard,
Rejecting RL fields, that's the card!
Tests confirm, both streamed and still,
Five hundred and one, by the rabbit's will. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is useful, but it does not follow the required template and omits the required Related Issues section. Add the required Overview, Details, Where should reviewer start?, and Related Issues sections, with either an issue link or the no-issue checkbox.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: rejecting unsupported RL fields for Responses.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@biswapanda biswapanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jthomson04
jthomson04 merged commit 769c6fc into main Jul 6, 2026
102 checks passed
@jthomson04
jthomson04 deleted the jthomson04/dyn-3392-reject-responses-rl-fields branch July 6, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants