[Bugfix][Frontend] Warn on silent GPT-OSS Harmony non-terminal parse drops - #45796
[Bugfix][Frontend] Warn on silent GPT-OSS Harmony non-terminal parse drops#45796Achyuthan-S wants to merge 1 commit into
Conversation
gpt-oss can emit malformed final turns that omit <|message|> ('...<|channel|>final {body}<|return|>'). In this case Harmony StreamableParser stays in HEADER, never commits final, and consumes the body as header tokens. HarmonyParser.parse() returns content=None, and serving emits finish_reason="stop" with content:null and billed tokens, silently dropping the answer.
Detect this at end-of-generation and log a warning when no content/tool calls were produced, the in-progress buffer is empty, and parser state is not EXPECT_START. The empty-buffer guard avoids flagging legitimate mid-content truncation. Adds regression test coverage. No response-shape change in this PR.
Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds visibility into malformed Harmony outputs where the assistant’s final channel is missing the <|message|> delimiter, preventing silent “empty” responses.
Changes:
- Import
StreamStateand add module logger for Harmony parser diagnostics. - Emit a warning when the parser ends in a non-terminal state with no content/tool calls produced.
- Add a regression test asserting the warning is logged for the malformed
final-missing-delimiter case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| vllm/parser/harmony.py | Adds non-terminal-state warning logic and supporting imports/logger. |
| tests/parser/test_harmony.py | Adds regression test validating the warning is emitted on malformed final output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from enum import Enum, auto | ||
| from typing import TYPE_CHECKING, NamedTuple | ||
|
|
||
| from openai_harmony import StreamState |
There was a problem hiding this comment.
This is a false positive. openai_harmony is a required dependency, not optional —
it's pinned in requirements/common.txt (openai-harmony >= 0.0.3 # Required for gpt-oss). This module already imports it unconditionally at runtime: harmony.py
imports harmony_utils, which does from openai_harmony import (... StreamableParser, load_harmony_encoding ...) at module top. So there's no environment where
harmony.py imports but openai_harmony is missing — the added StreamState import
introduces no new failure mode.
The suggested change also wouldn't work: StreamState is used at runtime
(self.state != StreamState.EXPECT_START), not just in an annotation, so a
TYPE_CHECKING-only import would raise NameError there. The runtime import matches
existing precedent in responses/context.py, which imports StreamState the same way
and compares against StreamState.EXPECT_START.
|
|
||
| @property | ||
| def state(self) -> HarmonyStreamState: | ||
| def state(self) -> StreamState: |
There was a problem hiding this comment.
Same concern as the thread on the from openai_harmony import StreamState line —
addressed there. Short version: openai_harmony is a required dep already imported
at runtime via harmony_utils, and StreamState is used at runtime
(StreamState.EXPECT_START), so it can't be TYPE_CHECKING-only.
Purpose
Fixes #45736.
In some cases gpt-oss emits a malformed final turn that skips the
<|message|>delimiter (
…<|channel|>final {body}<|return|>). In that case the HarmonyStreamableParsernever leaves the header state, never commits thefinalmessage, and treats the body as header tokens instead.
HarmonyParser.parse()then returnscontent=None, and vLLM surfaces this asfinish_reason="stop"withcontent: nulland non-zero completion tokens. Theanswer is effectively dropped, with no error and no log.
This change doesn’t change the HTTP response yet; it just makes this situation
visible in server logs.
What changed
In
HarmonyParser.parse(), after generation finishes, log a warning when:StreamState.EXPECT_START.The empty-buffer check avoids flagging cases where we intentionally truncate
mid-content and still have a recoverable partial body.
Added
test_malformed_final_missing_message_delimiterto cover the malformedfinal stream from the issue.
Scope
This PR is intentionally narrow:
content: nullandfinish_reason="stop"in this edge case.longer silent.
Changing the client-visible contract (e.g. different
finish_reason, or tryingto recover the trapped body) seems worth a separate discussion and follow-up PR.
Why this isn’t a duplicate
#43408 (“Tolerate malformed Harmony streams”) targeted streams where the
Harmony parser raises
HarmonyErrorand worked throughparse_output_into_messages, which was removed in the later Harmonyrefactors (#45171 / #45104).
This bug is different: the parser does not raise, it just ends in a
non-terminal state and drops content. The code paths and failure modes don’t
overlap with #43408.
Test plan