[BugFix] Add modalities and logprobs validation for /v1/chat/completions

[Shaun-Walsh]

Summary

• Validate modalities is a list of strings at handler level (ChatCompletionRequest is from upstream vllm, so a local Pydantic validator cannot be added)
• Validate logprobs is a boolean at handler level (Pydantic v2 lax mode coerces "yes" to True instead of returning 400)
• Apply the same raw-body validation to both single and batch chat-completion routes
• Preserve the existing default behavior for explicit JSON null values for modalities and logprobs
• Unskip modalities_list_bad_element and logprobs_wrong_type tests

Partial fix for #3649.

Approach

Since ChatCompletionRequest is imported from vllm.entrypoints.openai.chat_completion.protocol, we can't modify its Pydantic model. Instead, the raw JSON body is checked in the chat-completion entrypoints in api_server.py before dispatching to the serving layer. This keeps validation at the entrypoint level while preserving the upstream model's None defaults.

Test plan

• CPU TestClient coverage rejects modalities=[123] and logprobs="yes" before dispatch
• Manual local smoke checks covered the six malformed payload cases; this was not a live GPU run or a checked-in test artifact
• Build and pre-commit checks pass on the current PR head
• Live validation with Qwen3-Omni (requires 2x H100)

The live invalid-parameter coverage is marked slow + omni and uses two-card H100/MI325 hardware. It is collected by the weekly reliability selector:

pytest -s -v tests/dfx/reliability/invalid_param_test/ -m "slow and H100 and cards_2"

The manual checks above are not being used as a substitute for that live validation.

Remaining #3649 coverage

This PR only unskips the chat-completion cases. The following sibling invalid-parameter suites still contain intentional #3649 skips and are not claimed as fixed here:

• test_invalid_image_generation.py
• test_invalid_audio_speech.py
• test_invalid_audio_diffusion.py
• test_invalid_server_control.py

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[Shaun-Walsh]

Hi — just pushed a fix for the failing check. This PR (along with a few related ones from our team) is a dependency for the Red Hat Midstream build of vllm-omni. Could we get a review and merge when you have a chance? Happy to address any feedback quickly. Thanks!

[vllm-omni-review-bot]

Omni ReviewBot: no human activity for 48 days

@Shaun-Walsh this pull request has had no human commit, comment or review since 2026-07-21. Per repository policy it may be closed if it stays inactive.

To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline.

[Shaun-Walsh]

Hi @alex-jw-brooks @NickCao @yenuo26 just looking to get eyes this to get it reviewed

[herotai214]

@Shaun-Walsh — quick ask: could this wait until #5453 (P0.2 helper extract) merges?

The OmniAudioGenerate accessor already moved to vllm_omni/entrypoints/openai/app_state.py on #5453. A validation patch against the old api_server.py home will conflict. After #5453, please put that check on the remaining create_chat_completion route body in api_server.py (or on the accessor in app_state.py if that is the real target).

[Shaun-Walsh]

@Shaun-Walsh — quick ask: could this wait until #5453 (P0.2 helper extract) merges?

The OmniAudioGenerate accessor already moved to vllm_omni/entrypoints/openai/app_state.py on #5453. A validation patch against the old api_server.py home will conflict. After #5453, please put that check on the remaining create_chat_completion route body in api_server.py (or on the accessor in app_state.py if that is the real target).

Hi @herotai214 sure thing, I'll keep any eye on 5453 and we can comeback to this pr then :)

[yenuo26]

LGTM

[yenuo26]

@vllm-omni-review-bot

[vllm-omni-review-bot]

Omni ReviewBot routing record

Assigned Strict under experiment vllm-omni-strict-5050-20260829.

[vllm-omni-review-bot]

Omni ReviewBot review

Scan:

Category	Result
Tests / verification	4 finding(s) below
Security	1 finding(s) below
Docs / comments	1 finding(s) below
Behavior / compatibility	2 finding(s) below
Correctness	no finding reported

Validated:

• [resolved] double-json body hazard: cached body; residual: extra parse cost only.
• [claim-verified] PR claim "Partial fix for #3649": chat modalities_list_bad/logprobs_wrong_type unskipped; skips remain in test_invalid_image_generation.py, test_invalid_audio_speech.py, test_invalid_audio_diffusion.py, test_invalid_server_control.py.
• [claim-verified] Partial fix for #3649: chat unskips 2 cases; siblings still skip (_SKIP_ISSUE_3649 counts: audio_speech 27, audio_diffusion 8, image 6, server_control 4).
• [claim-refuted] PR "Verified validation logic locally (6/6)" — no in-repo evidence; covering cases are hardware slow+omni.
• [validated] Claim ChatCompletionRequest is upstream-imported: api_server.py:41-43 from vllm.entrypoints.openai.chat_completion.protocol.
• [validated] test asserts substrings matching new detail strings ("list of strings", "boolean") via send_chat_completions_http_request.

Merged outcome keeps four publishable threads on the primary fix: null present-keys now incorrectly 400 (major), merge-gate TestClient coverage for the new guards (major), batch sibling route still unguarded for the same #3649 coercion (minor), and explicit tracking of remaining _SKIP_ISSUE_3649 surfaces (minor), plus a nit on ErrorResponse envelope parity. Duplicate batch/test/scope/ErrorResponse samples were collapsed; affirmations, resolved #5453 notes, and rejected delete/consume candidates were dropped.

Checked, no defect found:

• tests/dfx/reliability/invalid_param_test/test_invalid_omni_chat.py:16 — No coverage hole after _SKIP_ISSUE_3649 removal: module skip constant is gone (file has zero _SKIP_ISSUE_3649 refs); unskipped modalities_list_bad / logprobs_wrong_type err_message substrings match api_server detail text; assert_http_error accepts FastAPI detail payloads. No further edit required.

Verdict: REQUEST CHANGES

Findings

• [P1] This diff unskips modalities_list_bad_element / logprobs_wrong_type in test_inv… — tests/dfx/reliability/invalid_param_test/test_invalid_omni_chat.py
  This diff unskips modalities_list_bad_element / logprobs_wrong_type in test_invalid_omni_chat.py (pytestmark slow+omni, hardware_test 2×H100), which weekly invalid_param_test selects—not merge/ready core_model+cpu jobs that cover api_server. Existing test_modality_validation assigns modalities on an already-built ChatCompletionRequest and never drives HTTP→Pydantic lax coercion through create_chat_completion. Add a core_model+cpu TestClient/handler test (alongside test_api_server_guards patterns) that POSTs modalities=[123] and logprobs="yes" and asserts 400 before merge so the new guards have a merge-gate regression.

Evidence: tests/dfx/reliability/invalid_param_test/test_invalid_omni_chat.py:16 pytestmark = [pytest.mark.slow, pytest.mark.omni]; :86 @hardware_test(res={"cuda": "H100", "rocm": "MI325"}, num_cards=2); :125 pytest.param("logprobs_wrong_type", 400, ("logprobs", "boolean"), id="logprobs_wrong_type"), (no skip). Unchanged by this diff, present in the PR-time tree: .buildkite/cuda/test-weekly.yml:58 - pytest -s -v tests/dfx/reliability/invalid_param_test/ -m "slow and H100 and cards_2"; .buildkite/cuda/test-ready.yml:28 - "pytest -sv tests/entrypoints tests/engine -m 'core_model and cpu'" (no invalid_param_test). Unchanged by this diff: tests/entrypoints/openai_api/test_modality_validation.py:54-61 _make_request builds ChatCompletionRequest(...) then req.modalities = modalities; :112-113 calls serving._create_chat_completion(request) — not api_server.create_chat_completion raw JSON. Unchanged by this diff: tests/entrypoints/openai_api/test_api_server_guards.py:59 pytestmark = [pytest.mark.core_model, pytest.mark.cpu] with no modalities/logprobs assertions. New guards (this diff): vllm_omni/entrypoints/openai/api_server.py:1352-1363 raw_body = await raw_request.json() then modalities/logprobs HTTPException 400 checks.

• **[P1] New raw-body checks fire on key presence (if "modalities" in raw_body / if "…** — vllm_omni/entrypoints/openai/api_server.py New raw-body checks fire on key presence (if "modalities" in raw_body/if "logprobs" in raw_body and not isinstance(..., bool)), so JSON null 400s before Omnichat—isinstance(None, list|bool) fails. Unchanged serving_chat still defaults null modalities (request.modalities = output_modalities if output_modalities is not None else engine_output_modalities`). Gate only when the value is not None so omit/null keep prior defaulting while still rejecting wrong types.

Evidence: api_server.py:1353-1363 (this PR) if "modalities" in raw_body: … if not isinstance(modalities, list) or not all(isinstance(m, str) for m in modalities): raise HTTPException(... BAD_REQUEST ...) and if "logprobs" in raw_body and not isinstance(raw_body["logprobs"], bool): raise HTTPException(... BAD_REQUEST ...) — null fails both isinstance checks. Unchanged by this diff, present in the PR-time tree: serving_chat.py:638-639 output_modalities = getattr(request, "modalities", engine_output_modalities) / request.modalities = output_modalities if output_modalities is not None else engine_output_modalities — proves None was previously defaulted, not rejected.

Suggestion: raw_body = await raw_request.json()
if "modalities" in raw_body and raw_body["modalities"] is not None:
modalities = raw_body["modalities"]
if not isinstance(modalities, list) or not all(isinstance(m, str) for m in modalities):
raise HTTPException(
status_code=HTTPStatus.BAD_REQUEST.value,
detail='modalities must be a list of strings, e.g. ["text", "audio", "image"]',
)
if (
"logprobs" in raw_body
and raw_body["logprobs"] is not None
and not isinstance(raw_body["logprobs"], bool)
):
raise HTTPException(
status_code=HTTPStatus.BAD_REQUEST.value,
detail="logprobs must be a boolean (true or false)",
)

• [P2] This diff adds raw-body modalities/logprobs isinstance guards only in create_ch… — vllm_omni/entrypoints/openai/api_server.py
  This diff adds raw-body modalities/logprobs isinstance guards only in create_chat_completion (api_server.py:1352–1364). create_batch_chat_completion (api_server.py:1440) has no equivalent; batch_serving.py:95–111 does request.to_chat_completion_request(msg) then asyncio.create_task(self.create_chat_completion(c, raw_request)) on already-typed ChatCompletionRequest objects, so /v1/chat/completions/batch never hits the new gate. Batch clients sending logprobs:"yes" or modalities:[123] still get the silent Pydantic lax coercion this PR targets for #3649. Extract a shared raw-body validator for both routes, or document batch as explicitly out of scope with a #3649 tracking note.

Evidence: PR-time tree (unchanged batch path outside the added chat-only guards): api_server.py:1352 raw_body = await raw_request.json() … api_server.py:1360 if "logprobs" in raw_body and not isinstance(raw_body["logprobs"], bool): — guards only on create_chat_completion; api_server.py:1440 async def create_batch_chat_completion(request: BatchChatCompletionRequest, raw_request: Request): then api_server.py:1441 handler = OmniBatchChat(raw_request) — no raw-body modalities/logprobs check; unchanged by this diff, present in PR-time tree: batch_serving.py:95 chat_cmp_request = request.to_chat_completion_request(msg) and batch_serving.py:111 tasks = [asyncio.create_task(self.create_chat_completion(c, raw_request)) for c in chat_requests] — fans out already-typed ChatCompletionRequest via serving method, not the guarded FastAPI route.

Suggestion: async def create_batch_chat_completion(request: BatchChatCompletionRequest, raw_request: Request):
raw_body = await raw_request.json()
if "modalities" in raw_body:
modalities = raw_body["modalities"]
if not isinstance(modalities, list) or not all(isinstance(m, str) for m in modalities):
raise HTTPException(
status_code=HTTPStatus.BAD_REQUEST.value,
detail='modalities must be a list of strings, e.g. ["text", "audio", "image"]',
)
if "logprobs" in raw_body and not isinstance(raw_body["logprobs"], bool):
raise HTTPException(
status_code=HTTPStatus.BAD_REQUEST.value,
detail="logprobs must be a boolean (true or false)",
)
handler = OmniBatchChat(raw_request)

• [P2] Chat modalities/logprobs invalid_param cases are unskipped, but sibling modules… — tests/dfx/reliability/invalid_param_test/test_invalid_omni_chat.py
  Chat modalities/logprobs invalid_param cases are unskipped, but sibling modules still define/use _SKIP_ISSUE_3649 (image generation, audio speech, audio diffusion, server control). List or link those remaining skips in the PR/issue so chat-only work is not mistaken for full #3649 closure.

Evidence: test_invalid_omni_chat.py:125 pytest.param("logprobs_wrong_type", 400, ("logprobs", "boolean"), id="logprobs_wrong_type"), (unskipped; _SKIP_ISSUE_3649 removed from this file). Unchanged by this diff, present in the PR-time tree: test_invalid_image_generation.py:16 _SKIP_ISSUE_3649 = pytest.mark.skip(reason="https://github.com/vllm-project/vllm-omni/issues/3649"); test_invalid_audio_speech.py:40 _SKIP_ISSUE_3649 = pytest.mark.skip(reason="https://github.com/vllm-project/vllm-omni/issues/3649"); test_invalid_audio_diffusion.py:17 _SKIP_ISSUE_3649 = pytest.mark.skip(reason="https://github.com/vllm-project/vllm-omni/issues/3649"); test_invalid_server_control.py:16 _SKIP_ISSUE_3649 = pytest.mark.skip(reason="https://github.com/vllm-project/vllm-omni/issues/3649").

• [P2] herotai214 #5453 placement: at this head create_chat_completion remains in vllm… — api_server.py:1351
  herotai214 #5453 placement: at this head create_chat_completion remains in vllm_omni/entrypoints/openai/api_server.py:1351 and owns the new modalities/logprobs checks (OmniAudioGenerate still api_server.py:1335).

Evidence: vllm_omni/entrypoints/openai/api_server.py:1335 def OmniAudioGenerate(request: Request) -> OmniOpenAIServingAudioGenerate | None:; vllm_omni/entrypoints/openai/api_server.py:1351 async def create_chat_completion(request: ChatCompletionRequest, raw_request: Request):; vllm_omni/entrypoints/openai/api_server.py:1352-1363 raw_body = await raw_request.json() then modalities list-of-strings and logprobs boolean HTTPException checks owned by that handler.

• [P2] vllm_omni/entrypoints/openai/serving_chat.py:641-642 already type-checks modali… — serving_chat.py:641
  vllm_omni/entrypoints/openai/serving_chat.py:641-642 already type-checks modalities post-parse; this PR’s raw-body check in api_server.py:1352-1358 is what rejects HTTP modalities:[123] with “list of strings”. Residual: create_batch_chat_completion (api_server.py:1440) has no raw-body guard; programmatic callers that set request.modalities after construction (test_modality_validation.py:60) still rely only on the post-parse check.

Evidence: unchanged by this diff, present in the PR-time tree: vllm_omni/entrypoints/openai/serving_chat.py:641-642 if not isinstance(request.modalities, list) or not all(isinstance(m, str) for m in request.modalities): / return self.create_error_response("'modalities' must be a list of strings.") — post-parse type check. In this PR diff: vllm_omni/entrypoints/openai/api_server.py:1352-1358 raw_body = await raw_request.json() … if not isinstance(modalities, list) or not all(isinstance(m, str) for m in modalities): / detail='modalities must be a list of strings, e.g. ["text", "audio", "image"]'. unchanged by this diff: vllm_omni/entrypoints/openai/api_server.py:1440 async def create_batch_chat_completion(request: BatchChatCompletionRequest, raw_request: Request): — no raw_body modalities guard. unchanged by this diff: tests/entrypoints/openai_api/test_modality_validation.py:60 req.modalities = modalities and :110 request = _make_request(["text", 123]) — programmatic path relies on serving_chat post-parse check. In this PR diff: tests/dfx/reliability/invalid_param_test/test_invalid_omni_chat.py:63 body["modalities"] = [123] with expected substring "list of strings".

• [P2] create_chat_completion calls await raw_request.json() after FastAPI already bou… — ``
  create_chat_completion calls await raw_request.json() after FastAPI already bound ChatCompletionRequest; Starlette caches _body so this is not a stream double-consume hazard — residual is one extra json.loads only.

Evidence: vllm_omni/entrypoints/openai/api_server.py:1351-1352 async def create_chat_completion(request: ChatCompletionRequest, raw_request: Request): / raw_body = await raw_request.json() — second JSON read after FastAPI already injected the Pydantic body. unchanged by this diff, installed dependency starlette/requests.py:254-266 async def body(self) -> bytes: if not hasattr(self, "_body"): ... self._body = b"".join(chunks) / async def json(self) -> Any: if not hasattr(self, "_json"): body = await self.body(); self._json = json.loads(body) — body bytes are cached; json() only re-parses.

[vllm-omni-review-bot]

Omni ReviewBot review

PR description

This change adds a small raw-JSON check on the Omni chat-completion HTTP handlers so modalities must be a list of strings and logprobs must be a real JSON boolean. That rejects payloads such as modalities=[123] and logprobs="yes" with HTTP 400 before the upstream ChatCompletionRequest model can coerce them. Explicit JSON null for either field is still treated as unset. The same helper is invoked from both /v1/chat/completions and /v1/chat/completions/batch, and the previously skipped chat invalid-parameter cases are left enabled.

Change flow

flowchart TD
  A["[EXISTING] Client JSON to /v1/chat/completions or /batch"]:::existing
  B["[CHANGED] create_chat_completion reads raw_request.json"]:::changed
  C["[CHANGED] create_batch_chat_completion reads raw_request.json"]:::changed
  D["[NEW] _validate_chat_completion_raw_body"]:::new
  E["[EXISTING] HTTP 400 for bad list/bool types"]:::existing
  F["[EXISTING] Unset/null continues to Omni chat serving"]:::existing
  A --> B
  A --> C
  B --> D
  C --> D
  D --> E
  D --> F
  classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
  classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
  classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
  classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px

Loading

No actionable findings.

[alex-jw-brooks]

Thanks @Shaun-Walsh!