Repository navigation
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb7cdb959c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 6b2324d7b7d087711486692181b8730cb24522e8 and a1fe257474af168fdfa9d9e132e89ac2ab22f2c0. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughParserEngine now withholds incomplete trailing JSON values, avoids partial escapes, and completes safe prefixes during flushes. Parser tests cover prefix handling and terminators. Qwen3 tests cover unclosed and truncated string and typed parameters. ChangesStreaming JSON argument handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Qwen3Parser
participant ParserEngine
participant JSONDecoder
Qwen3Parser->>ParserEngine: finish_streaming()
ParserEngine->>ParserEngine: _json_prefix_terminator()
ParserEngine->>JSONDecoder: validate completed JSON prefix
JSONDecoder-->>ParserEngine: valid JSON result
ParserEngine-->>Qwen3Parser: flushed argument delta
Merge Risk: ⚪ Minimal · up to The change completes truncated streamed tool-call JSON arguments and includes reported parser and Qwen3 regression coverage. No concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The fix is well-scoped, includes deterministic regression coverage, and the remaining feedback is limited to a documentation clarification.
Pull request overview
Fixes a ParserEngine streaming edge case where tool-call arguments can be left as an unterminated JSON prefix when generation ends mid-parameter, by terminating the already-streamed prefix during the end-of-stream flush so clients can successfully json.loads the accumulated arguments.
Changes:
- Update
_safe_arg_prefixto avoid streaming a trailing non-streamable key’s dangling separator (so the flush can safely drop an unfinished parameter). - Extend
_flush_arg_converterwith_close_streamed_json+_json_prefix_terminatorto append the shortest suffix that makes the streamed prefix parseable. - Add targeted regression tests for Qwen3 streaming truncation/unclosed-parameter cases and unit tests for the new prefix-termination logic.
File summaries
| File | Description |
|---|---|
vllm/parser/engine/parser_engine.py |
Ensures streamed tool-call argument prefixes are terminated into parseable JSON when the flush cannot extend them. |
tests/parser/engine/test_qwen3.py |
Adds streaming regression tests for truncated/unclosed Qwen3 tool parameters (including typed-schema behavior). |
tests/parser/engine/test_parser_engine.py |
Updates _safe_arg_prefix expectations and adds unit tests for _json_prefix_terminator. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Follow-up at 5448bf23a, 6b2324d7b and a1fe2574 for the four review findings. The two substantive ones were real and are fixed at the source. Red/green on kebab-spark-3 (GB10, vllm 0.26.1rc1.dev1193+gc2294420c): with the source change reverted, 17 of the 37 cases in Test docstrings shortened as asked, and the |
…rom the streamed prefix
The terminator added in c2294420c could only close a prefix that ended
inside a string. _safe_arg_prefix also streams a non-streamable trailing
value as its separator alone, so a truncation inside a schema-typed
number, bool, object or array left the client holding
'{"path": "README.md", "count": ' with no valid completion (Codex P1 on
vllm-project#53739). A streamed string could likewise end inside a \uXXXX escape,
which no suffix can complete without inventing characters (Codex P2).
Two layers:
- _safe_arg_prefix stops after the last complete top-level value when the
trailing value cannot stream, and never ends a streamed string inside an
escape sequence. Existing TestSafeArgPrefix expectations that encoded the
dangling '"key": ' shape are updated; the streaming contract (prefix
monotonic, flush extends) is unchanged.
- _json_prefix_terminator completes a dangling ':' or an array ',' with
null, the one placeholder every schema type accepts, and still yields no
suffix for shapes it cannot close (object after a comma, partial escape,
partial literal); the json.loads check remains the final gate.
Tests: TestSafeArgPrefix extended, TestJsonPrefixTerminator added, and a
streaming regression for a truncated schema-typed parameter. With the
source change reverted, 17 of the 37 cases in those classes fail; with it,
tests/parser/engine is 3814 passed (kebab-spark-3, GB10,
vllm 0.26.1rc1.dev1193+gc2294420c).
Signed-off-by: Alex Mittell <mittell@me.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
(cherry picked from commit 9037f20)
Per the review on vllm-project#53739: the tests' names and assertions carry the behavior; the mechanism is documented on _close_streamed_json. Signed-off-by: Alex Mittell <mittell@me.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 856b2c1)
…lidity Review on vllm-project#53739: JSON Schema rejects `null` unless the type admits it, so "the one placeholder every schema type accepts" was wrong. The comment now says what the placeholder is actually for: making the prefix PARSE. The client is being handed a truncated call either way, and a null that fails the tool's schema is strictly better than arguments no JSON parser accepts. Comment only, no behaviour change. tests/parser/engine: 3,814 passed on kebab-spark-3 (GB10). Signed-off-by: Alex Mittell <mittell@me.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 870edc8)
…rom the streamed prefix
The terminator added in c2294420c could only close a prefix that ended
inside a string. _safe_arg_prefix also streams a non-streamable trailing
value as its separator alone, so a truncation inside a schema-typed
number, bool, object or array left the client holding
'{"path": "README.md", "count": ' with no valid completion (Codex P1 on
vllm-project#53739). A streamed string could likewise end inside a \uXXXX escape,
which no suffix can complete without inventing characters (Codex P2).
Two layers:
- _safe_arg_prefix stops after the last complete top-level value when the
trailing value cannot stream, and never ends a streamed string inside an
escape sequence. Existing TestSafeArgPrefix expectations that encoded the
dangling '"key": ' shape are updated; the streaming contract (prefix
monotonic, flush extends) is unchanged.
- _json_prefix_terminator completes a dangling ':' or an array ',' with
null, the one placeholder every schema type accepts, and still yields no
suffix for shapes it cannot close (object after a comma, partial escape,
partial literal); the json.loads check remains the final gate.
Tests: TestSafeArgPrefix extended, TestJsonPrefixTerminator added, and a
streaming regression for a truncated schema-typed parameter. With the
source change reverted, 17 of the 37 cases in those classes fail; with it,
tests/parser/engine is 3814 passed (kebab-spark-3, GB10,
vllm 0.26.1rc1.dev1193+gc2294420c).
Signed-off-by: Alex Mittell <mittell@me.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Per the review on vllm-project#53739: the tests' names and assertions carry the behavior; the mechanism is documented on _close_streamed_json. Signed-off-by: Alex Mittell <mittell@me.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lidity Review on vllm-project#53739: JSON Schema rejects `null` unless the type admits it, so "the one placeholder every schema type accepts" was wrong. The comment now says what the placeholder is actually for: making the prefix PARSE. The client is being handed a truncated call either way, and a null that fails the tool's schema is strictly better than arguments no JSON parser accepts. Comment only, no behaviour change. tests/parser/engine: 3,814 passed on kebab-spark-3 (GB10). Signed-off-by: Alex Mittell <mittell@me.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a1fe257 to
af6a082
Compare
|
Pushed
Well-formed calls through the Qwen3 and Inkling parsers, 549 feed variants: arguments are identical to The #48706 section of the description is updated to match. |
|
Pushed
The coercion now rewrites only the coerced top-level values, in place. 11 inputs: 5 ended wrong at Description changes:
|
|
Scope, now that the per-parser PRs are up: #57707, #59882 and #58349/#58418/#58437 fix the qwen3, minimax_m2, glm47 and deepseek_v4 parsers one at a time. This PR covers what they don't: converters that pass the model's JSON through verbatim (Inkling, Granite), and keeping the streamed argument prefix consistent with the final arguments. It merges cleanly after #57707 or #59882. @chaunceyjiang @sfeng33 could one of you take a look? |
…rom the streamed prefix
The terminator added in c2294420c could only close a prefix that ended
inside a string. _safe_arg_prefix also streams a non-streamable trailing
value as its separator alone, so a truncation inside a schema-typed
number, bool, object or array left the client holding
'{"path": "README.md", "count": ' with no valid completion (Codex P1 on
vllm-project#53739). A streamed string could likewise end inside a \uXXXX escape,
which no suffix can complete without inventing characters (Codex P2).
Two layers:
- _safe_arg_prefix stops after the last complete top-level value when the
trailing value cannot stream, and never ends a streamed string inside an
escape sequence. Existing TestSafeArgPrefix expectations that encoded the
dangling '"key": ' shape are updated; the streaming contract (prefix
monotonic, flush extends) is unchanged.
- _json_prefix_terminator completes a dangling ':' or an array ',' with
null, the one placeholder every schema type accepts, and still yields no
suffix for shapes it cannot close (object after a comma, partial escape,
partial literal); the json.loads check remains the final gate.
Tests: TestSafeArgPrefix extended, TestJsonPrefixTerminator added, and a
streaming regression for a truncated schema-typed parameter. With the
source change reverted, 17 of the 37 cases in those classes fail; with it,
tests/parser/engine is 3814 passed (kebab-spark-3, GB10,
vllm 0.26.1rc1.dev1193+gc2294420c).
Signed-off-by: Alex Mittell <mittell@me.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Per the review on vllm-project#53739: the tests' names and assertions carry the behavior; the mechanism is documented on _close_streamed_json. Signed-off-by: Alex Mittell <mittell@me.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lidity Review on vllm-project#53739: JSON Schema rejects `null` unless the type admits it, so "the one placeholder every schema type accepts" was wrong. The comment now says what the placeholder is actually for: making the prefix PARSE. The client is being handed a truncated call either way, and a null that fails the tool's schema is strictly better than arguments no JSON parser accepts. Comment only, no behaviour change. tests/parser/engine: 3,814 passed on kebab-spark-3 (GB10). Signed-off-by: Alex Mittell <mittell@me.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Carry the coercion-instability fix from vllm-project#48706 onto the streamed-argument termination work in vllm-project#53739. Resolve the overlapping _safe_arg_prefix changes by clipping a non-string value only after a top-level comma proves it is a middle field, while preserving the trailing-field withholding behavior. Co-authored-by: mahadrehmann <mahadrehman04@gmail.com> Co-authored-by: muhammadfawaz1 <135441198+muhammadfawaz1@users.noreply.github.com> Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: ZhengDe_L <245653240+ZhengDeL@users.noreply.github.com> (cherry picked from commit 0092327) Signed-off-by: Alex Mittell <mittell@me.com>
9e14c0c to
c30a211
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
…xtend Engine-based parsers stream unterminated string values for prefix-stable keys, on the contract that _flush_arg_converter completes them at tool end. When generation ends while a parameter is still open, the non-partial re-derivation drops that parameter, so the flush result is neither longer than nor a prefix of what was already streamed and nothing is emitted. The stream then ends with finish_reason set while arguments is cut off mid-string, which no JSON parser accepts. Close the prefix that was already sent instead of emitting nothing. The client gets a truncated value rather than an unparseable message. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Alex Mittell <lamprey.64.veils@icloud.com>
The repo's `typos` pre-commit hook (crate-ci/typos v1.43.5) rejects
`unparseable`:
error: `unparseable` should be `unparsable`
Verified by running the hook both ways on the two affected files: it fails
on the current spelling and passes after this change. The rest of the tree
already uses `unparsable` in six files, including
vllm/entrypoints/chat_utils.py and tests/parser/mistral/test_tool_calls.py,
so this is the established spelling rather than a preference.
Docstrings only; no behavior change.
Signed-off-by: Alex Mittell <mittell@gmail.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…rom the streamed prefix
The terminator added in c2294420c could only close a prefix that ended
inside a string. _safe_arg_prefix also streams a non-streamable trailing
value as its separator alone, so a truncation inside a schema-typed
number, bool, object or array left the client holding
'{"path": "README.md", "count": ' with no valid completion (Codex P1 on
vllm-project#53739). A streamed string could likewise end inside a \uXXXX escape,
which no suffix can complete without inventing characters (Codex P2).
Two layers:
- _safe_arg_prefix stops after the last complete top-level value when the
trailing value cannot stream, and never ends a streamed string inside an
escape sequence. Existing TestSafeArgPrefix expectations that encoded the
dangling '"key": ' shape are updated; the streaming contract (prefix
monotonic, flush extends) is unchanged.
- _json_prefix_terminator completes a dangling ':' or an array ',' with
null, the one placeholder every schema type accepts, and still yields no
suffix for shapes it cannot close (object after a comma, partial escape,
partial literal); the json.loads check remains the final gate.
Tests: TestSafeArgPrefix extended, TestJsonPrefixTerminator added, and a
streaming regression for a truncated schema-typed parameter. With the
source change reverted, 17 of the 37 cases in those classes fail; with it,
tests/parser/engine is 3814 passed (kebab-spark-3, GB10,
vllm 0.26.1rc1.dev1193+gc2294420c).
Signed-off-by: Alex Mittell <mittell@me.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Per the review on vllm-project#53739: the tests' names and assertions carry the behavior; the mechanism is documented on _close_streamed_json. Signed-off-by: Alex Mittell <mittell@me.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lidity Review on vllm-project#53739: JSON Schema rejects `null` unless the type admits it, so "the one placeholder every schema type accepts" was wrong. The comment now says what the placeholder is actually for: making the prefix PARSE. The client is being handed a truncated call either way, and a null that fails the tool's schema is strictly better than arguments no JSON parser accepts. Comment only, no behaviour change. tests/parser/engine: 3,814 passed on kebab-spark-3 (GB10). Signed-off-by: Alex Mittell <mittell@me.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ches The docstring-coverage check on this PR flagged the touched functions at 40%. _flush_arg_converter, the two new fixtures and the three unit tests had none; the comment on the schema-typed test becomes its docstring. No behaviour change. pytest tests/parser/: 4744 passed (macOS arm64, CPU only). Co-authored-by: Claude Signed-off-by: Alex Mittell <mittell@me.com>
… open
When generation stops inside the args object, _inkling_arg_converter
returns the cut-off span verbatim even with partial=False. The flush then
extends the streamed prefix with that span without checking that it
parses. With a {s: string, n: integer} schema and EOS after
`"s": "hello", "n": 4`, the client streams `{"s": "hello"`, the flush
appends `, "n": 4`, and the client is left holding `{"s": "hello", "n": 4`.
In the extend branch, when final_json does not parse, append
_json_prefix_terminator(final_json) to the diff. A final_json that parses
is sent exactly as before, and a prefix the terminator cannot complete gets
"" and keeps the old behavior. The other arg converters (qwen3 and
seed_oss, minimax_m2, glm47, dsml, gemma4) build their result with
json.dumps, so they never reach this path.
Found in review; fix shape from review. The new tests feed the real Inkling
parser one token per delta, and fail on the parent commit (6 failed) and
pass here (6 passed). Well-formed calls stream byte-identical arguments
before and after across 2,730 feed variants: whole, one token or character
per delta, and every split point, 826 Qwen3 and 1,904 Inkling.
pytest tests/parser/: 4744 passed before, 4750 after (macOS arm64, CPU only).
Co-authored-by: Claude
Signed-off-by: Alex Mittell <mittell@me.com>
…as no completion
The previous commit closes an unterminated final_json in the flush's
extend branch. Where _json_prefix_terminator has no completion for it,
the unparsable extension was still sent. For Inkling that happens when EOS
lands inside or just after a key, right after a comma, inside a literal
such as `tr` or a bare `-`, or inside a `\u` escape. With
`{"s": "hello", ` the client ended on `{"s": "hello", `.
In that case, drop the extension and close the prefix already streamed
with _close_streamed_json, the same as the branch above it does. The
client gets `{"s": "hello"}`.
EOS at every character inside six well-formed Inkling args objects, 209
cut points: unparsable arguments 159 at 462f6636ff, 74 after the previous
commit, 0 here. 191 are valid JSON. The other 18 stop inside the first key,
before any argument text was streamed, and now stream no arguments rather
than a fragment such as `{"s`.
Six new regression shapes, each at EOS and at <|end_message|>: all 12 fail
on the parent commit and pass here. Well-formed calls stream byte-identical
arguments across the same 2,730 feed variants.
pytest tests/parser/: 4750 passed before, 4762 after (macOS arm64, CPU only).
Co-authored-by: Claude
Signed-off-by: Alex Mittell <mittell@me.com>
Carry the coercion-instability fix from vllm-project#48706 onto the streamed-argument termination work in vllm-project#53739. Resolve the overlapping _safe_arg_prefix changes by clipping a non-string value only after a top-level comma proves it is a middle field, while preserving the trailing-field withholding behavior. Co-authored-by: mahadrehmann <mahadrehman04@gmail.com> Co-authored-by: muhammadfawaz1 <135441198+muhammadfawaz1@users.noreply.github.com> Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: ZhengDe_L <245653240+ZhengDeL@users.noreply.github.com> (cherry picked from commit 0092327) Signed-off-by: Alex Mittell <mittell@me.com>
The previous commit withholds a non-string middle value, so the streamed
prefix can end on that field's `"key": ` separator. If EOS then lands
where the flush's span has no completion (inside a key, after a comma,
inside a literal), closing the streamed prefix fills that separator with
`null`, although the model had finished the value. Through InklingParser,
`{"n": 42, ` ended `{"n": null}`.
When the span has no completion, close it after its last complete
top-level value if that extends what was streamed, and fall back to the
streamed prefix only otherwise. `{"n": 42, ` ends `{"n": 42}` again.
EOS at every character of two Inkling args objects with a non-string field
first, 90 cut points: 27 replaced a completed value with null on the
parent commit (11 of those also dropped a later field), 0 here and 0 on
b3fbaf77d4. Three new TestTruncatedArgsFlush shapes, each at EOS and at
<|end_message|>: all 6 fail on the parent commit and pass here.
pytest tests/parser/engine: 4365 passed on the parent, 4371 here
(Linux x86_64, CPU only).
Co-authored-by: Claude
Signed-off-by: Alex Mittell <mittell@me.com>
…s not parse
Withholding every non-string middle value stops a call streaming at its
first non-string field. That is only needed when _fix_arg_types could not
coerce the value while streaming, which is when the converter's partial
output does not parse: inkling's verbatim span, or the tests'
_json_converter. The other in-tree converters build their output with
json.dumps on every tick, so their middle values are already coerced.
Clip only when the tick's JSON does not parse. Through a Qwen3
ParserEngine with a schema, an integer then a 27-chunk string: 1 argument
delta before </parameter> on the parent commit, 28 here, as on
b3fbaf77d4. An Inkling call with a quoted integer still ends
{"n": 42, "s": "hi"}.
test_qwen3_partial_xml_close_tag_not_included_in_value is replaced. It fed
a partial </parameter> straight into an ARG_VALUE_CHUNK, which the Qwen3
scanner does not emit (test_streaming_long_string_arg_before_parameter_end
splits the close tag the same way and streams no tag text), and it passed
only because the integer clip withheld the string after it. Its
replacement drives the Qwen3 grammar with a schema: an integer, then a
string whose close tag is split, must stream the string before the tag and
end with the integer coerced. It fails on the parent commit and passes
here. Also restores the import order ruff's isort check expects in
test_parser_engine.py.
Well-formed calls through the Qwen3 and Inkling parsers, 549 feed
variants: arguments identical to b3fbaf77d4 in 539. The other 10 are the
quoted-integer Inkling input, which now ends {"n": 42, "content": "x"}
instead of {"n": "42", "content": "x"}.
pytest tests/parser/engine: 4371 passed (Linux x86_64, CPU only).
Co-authored-by: Claude
Signed-off-by: Alex Mittell <mittell@me.com>
_fix_arg_types re-serialized the whole object with json.dumps whenever it
coerced any value. Inkling passes the model's JSON through verbatim, so a
streamed prefix with compact separators, a \u00e9 escape or newline
indentation stops being a prefix of the flush result, and the flush closes
what was streamed instead. Through InklingParser, one token per delta, with
n: integer, {"s":"a","n":"42","t":"b"} ended {"s":"a","n":null} and
{"s":"a","t":"b","n":"42"} ended {"s":"a","t":"b"}, both with no error.
Re-serialize only the coerced top-level values, in place, and keep every
other character. For converters that already build their output with
json.dumps(..., ensure_ascii=False) the result is byte-identical to the old
one: 60,000 randomized inputs over 3 seeds, 0 differences.
11 coercion inputs through InklingParser: 5 ended wrong on the parent (compact
middle and last field, a \u00e9 escape with compact or spaced separators,
newline-indented JSON), 0 here, and the streamed arguments now match the
non-streaming result byte for byte in all 11. Granite, added on main in
vllm-project#49648 with the same verbatim converter, gives the same 5 on the parent
merged with main, and 0 with this commit merged with main.
test_coercion_keeps_the_streamed_prefix (5 shapes) and
test_only_coerced_values_are_rewritten: 5 of the 6 fail on the parent. The
compact_first shape passes there too, because the streamed prefix stops at
{"n": and json.dumps writes {"n": 42. Also brings the TestJsonPrefixTerminator
comment in line with the source: null is there so the prefix parses, and it
may still fail the schema.
pytest tests/parser: 4772 passed on the parent, 4778 here
(Linux x86_64, CPU only).
Co-authored-by: Claude
Signed-off-by: Alex Mittell <mittell@me.com>
c30a211 to
9f921e8
Compare
Purpose
A streaming tool call can end with
finish_reasonset whileargumentsis cut off mid-string. Any client that callsjson.loadson the accumulated arguments fails on it. Reproduced live on Qwen3.8-27B in 7 of 25 requests sent straight at the worker, no proxy in the path._safe_arg_prefix(parser_engine.py:299on main at8a2364605c0b) deliberately streams unterminated string values for prefix-stable keys. Its own docstring (:306-308) carries the contract: "String values for keys instring_keysare prefix-stable, so stream their unterminated content instead of buffering long arguments until the closing tag arrives." Something has to close them, and that something is the flush._flush_arg_converter(parser_engine.py:1052on main) re-derives the arguments withpartial=False. Four of the seven in-tree converters build their result from completed parameters and fold the still-open one in only underif partial:. So when generation ends while a parameter is open, the non-partial re-derivation drops that parameter, and the result is neither longer than nor a prefix of what was already streamed. Both guards (:1068-1070) fail, the function returnsNone, and nothing ever terminates the prefix the client is holding.This is not conditional on the model misbehaving. Anything that ends generation with a parameter open reaches it: a
max_tokensstop, a stop string, or a malformed continuation.Blast radius, measured
On main at
8a2364605c0b, 13 engine parsers set anarg_converter, through 7 converters. Each converter called directly with one complete and one open string parameter:partial=True->partial=False_qwen3_arg_converter_minimax_m2_arg_converter_glm47_arg_converter_dsml_arg_converter_inkling_arg_converter_granite_arg_converter_gemma4_arg_converter12 of the 13 parsers use a converter that reaches the defect. gemma4 does not, which is why the fix sits at the engine's guard rather than in any one converter. kimi_k2 and mistral set no
arg_converter, return early and are unaffected.End to end through one parser per converter, EOS at every token of a call whose last string parameter is open, one token per delta: 62 of 98 cuts end with unparsable
argumentsfor qwen3, 59 of 109 for minimax_m2, 59 of 130 for glm47_moe, 65 of 155 for deepseek_v4, and 73 of 73 for both inkling and granite. With this PR merged into main, none of the six ends on partial JSON; inkling and granite end with no arguments at all on 6 cuts each (known limit below).The two shapes need two branches, and both are in this change: the four dropping converters fail the
startswithguard, and inkling and granite fail the length guard by returning exactly what was already sent.The fix
Close what was already streamed instead of emitting nothing.
_json_prefix_terminatorscans the prefix for an open escape, an open string and unbalanced brackets, appends the shortest suffix that makes it parse, and verifies withjson.loadsbefore returning it. If it cannot make the prefix parse it returns""and the oldNonebehavior stands, so this can only turn an unparsable message into a parseable one.The client gets the truncated value rather than an unparsable message. Truncation is already the semantics here, since the model never produced the rest.
Known limits. EOS inside the first key of an inkling or granite args object (
{"pa) streams no arguments at all, 6 of the 73 cuts above; on main those 6 stream the partial key and do not parse either. And this PR changes only the stream: a truncated inkling or granite call still returns the unterminated span as its non-streamingarguments({"path": "README.md", "content": "# Title...), on main, on main with the four converter PRs below, and with this PR.Coercion must not rewrite the streamed prefix
_fix_arg_types(parser_engine.py:381on main) re-serialized the whole object withjson.dumps(:405) whenever it coerced any value. inkling and granite pass the model's JSON through verbatim, so a streamed prefix with compact separators, a\u00e9escape or newline indentation stopped being a prefix of the flush result, and the flush closed what was streamed instead. At9b25b6123cthat lost data with no error. It now re-serializes only the coerced top-level values, in place, and keeps every other character.Through
InklingParser, one token per delta, schemas: string, n: integer, t: string:8a2364605c0b9b25b6123c9c69f476174d{"s":"a","n":"42","t":"b"}{"s":"a","n":"42","t":"b(unparsable){"s":"a","n":null}{"s":"a","n":42,"t":"b"}{"s":"a","t":"b","n":"42"}{"s":"a","t":"b","n":(unparsable){"s":"a","t":"b"}{"s":"a","t":"b","n":42}{"s": "\u00e9", "n": "42", "t": "b"}{"s": "\u00e9", "n": null}{"s": "\u00e9", "n": 42, "t": "b"}{"s": "a", "n": "42", "t": "b"}{"s": "a", "n": 42, "t": "b"}{"s": "a", "n": 42, "t": "b"}Over 11 such inputs, 5 ended wrong at
9b25b6123cand 0 at9c69f476174d, where the streamed arguments also match the non-streaming result byte for byte in all 11. Granite, merged with main, gives the same 5 and 0. Thenullarrived with the #48706 port in4aacd85b9a: atb3fbaf77d4the compact middle case ended{"s":"a","n":"42","t":"b"}, parseable, withnstill a string. The dropped last field dates to67f2f7c13f.For converters that already build their output with
json.dumps(..., ensure_ascii=False)(qwen3, minimax_m2, glm47, dsml, gemma4) the new output is byte-identical to the old one: 60,000 randomized inputs over 3 seeds, 0 differences. For inkling and granite, non-streamingargumentsnow keep the model's own formatting when a value is coerced, as they already did when nothing was coerced.Relationship to other work
partial=Falsepass keep the open trailing parameter, for qwen3, minimax_m2, glm47 and dsml (deepseek_v4, v32, v41). That is a converter-level fix; this PR is engine-level. For those four converters the two overlap, and the converter fixes keep more: a truncated"count": 12survives as12, where this PR alone withholds it, and deepseek_v4's open string survives in full, where this PR alone closes it as"". They also fix the non-streaming result, which this PR does not touch: with this PR alone, non-streamingargumentsfor those four still drop the open parameter ({"path": "README.md"}) while the stream carries its truncated content. I applied all four to main8a2364605c0bwithout this PR's source: EOS at every token ends parseable for qwen3, minimax_m2, glm47_moe and deepseek_v4 (0 unparsable in 98, 109, 130 and 155 cuts), and this PR's four Qwen3 tests pass. What they cannot reach is a converter that returns the model's JSON verbatim. On that tree inkling and granite stay at 73 of 73 unparsable, and 32 of this PR's tests fail: all 29 cases inTestTruncatedArgsFlush(24 truncation, 5 coercion) and the three [Bugfix][Parser] Fix streaming tool-call args truncated by schema coercion #48706 middle-field tests. The dangling"key":and the partial\uescape are withheld in_safe_arg_prefix, and in these runs only inkling and granite need that: for qwen3, minimax_m2 and glm47 the converter PRs alone also fix a truncated integer and a truncated"ok": tr, andjson.dumpsoutput never ends inside an escape. With this PR and all four merged into main,pytest tests/parsergives 5211 passed, and the 12 qwen3, minimax_m2, glm47_moe and deepseek_v4 outputs are byte-identical to the four PRs alone.b33c8a657d48. On main,serving.py:765-770reportstool_callsonly when the engine's own finish reason isstop, so amax_tokenscutoff now keepslength. A stop string still reportstool_calls. It changes onlychat_completion/serving.pyand its test, so the payload is still unparsable on main without this PR, as the sweep above shows. All 7 of my field captures predate it and reportfinish_reason: tool_calls._safe_arg_prefix: a non-string middle field is coerced at flush time and breaks thestartswithinvariant. This PR now carries that fix. Both PRs change_safe_arg_prefix, and [Bugfix][Parser] Fix streaming tool-call args truncated by schema coercion #48706's unconditional clip, applied here, fails 5 of this PR's tests (theliteralandsigncases ofTestTruncatedArgsFlush, at EOS and at<|end_message|>, plustest_schema_typed_trailing_string_is_withheld_with_its_key). ZhengDeL ported it onto this branch in [Bugfix][Parser] Integrate coercion-safe prefixes with streamed-argument termination amittell/vllm#2, adding alast_commacondition that keeps a trailing field withheld along with its key. That port is4aacd85b9a, with his authorship and mahadrehmann and muhammadfawaz1 as co-authors. On top of it,90105e1762stops an interrupted call from closing a finished field asnull,9b25b6123cwithholds middle values only when the tick's JSON does not parse, so Qwen3 and the otherjson.dumpsconverters keep streaming after a non-string field, and9c69f476174dstops the flush's coercion from rewriting what inkling and granite already streamed. With this PR merged, [Bugfix][Parser] Fix streaming tool-call args truncated by schema coercion #48706's fix is in, along with three of its four tests. The fourth, the Qwen3 close-tag test, is replaced as described under Test Plan.<tool_call>opener (ParserEngine / Qwen3) #47137 (closed) documented the same trigger class from the other side, content/markup divergence when generation terminates viamax_tokensor a stop string before the tool call is promoted. Different code path, same class of cutoff.qwen3_coderon the ParserEngine path. I am not claiming it: the reporter saw it on v0.23.0 as well as v0.24.0, andvllm/parser/engine/parser_engine.pydoes not exist at v0.23.0, so at least part of their report has a different cause.Not a duplicate. Searched open PRs on 2026-09-25 for
_flush_arg_converter,streamed_json,_safe_arg_prefix,_fix_arg_types, "unterminated tool call arguments", "tool arguments unterminated", "parser engine flush arguments" and "streamed tool arguments". Besides the PRs above, #55497 adds a warning log in the same_flush_arg_converterbranch and leaves the prefix unterminated, and #51141 makes_safe_arg_prefixincremental for performance. Both conflict textually with this PR (git merge-treeagainst9c69f476174d: #55497 inparser_engine.py, #51141 inparser_engine.pyand both test files); neither closes the prefix or touches the coercion re-serialization.Test Plan
Two regressions in
tests/parser/engine/test_qwen3.py, one per trigger:test_unclosed_parameter_still_streams_valid_json- the model reopens<tool_call>without closing the current parameter.test_truncated_parameter_still_streams_valid_json- generation simply stops mid-value, no stray markup at all.Both feed one character at a time, which is the worst case for the prefix invariant and the only way to hit the boundary reliably. An earlier version of the first test used natural chunk boundaries and passed with the fix reverted, because
<tool_call>arrived as a complete literal and the incremental lexer transitioned cleanly.Sweeping every single split point of both inputs makes the boundary explicit:
<tool_call>In both cases the 12 reproducing boundaries are exactly positions 1 to 12, inside the opening
<tool_call>\nliteral, which is what forces the lexer to hold the prefix across the chunk edge. Char-by-char feeding reproduces on both and is what the committed tests use.Coercion-safe prefixes
TestCoercionInstabilityRegression(tests/parser/engine/test_parser_engine.py): integer, boolean and null middle fields must end coerced and untruncated. All 3 fail atb3fbaf77d4.TestTruncatedArgsFlush(tests/parser/engine/test_inkling.py) gains three shapes with a non-string field first:comma_after_number,literal_after_numberandliteral_after_string_after_number, each at EOS and at<|end_message|>. All 6 fail at4aacd85b9aand pass at90105e1762.test_streaming_string_after_int_param_streams_before_close(tests/parser/engine/test_qwen3.py) goes through the Qwen3 grammar. It fails at90105e1762and passes at9b25b6123c.TestTruncatedArgsFlush::test_coercion_keeps_the_streamed_prefix(5 shapes: compact first, middle and last field, a\u00e9escape, newline-indented JSON) andTestFixArgTypes::test_only_coerced_values_are_rewritten: 5 of the 6 fail at9b25b6123cand all pass at9c69f476174d.compact_firstpasses at9b25b6123ctoo, because the streamed prefix stops at{"n":andjson.dumpswrites{"n": 42, so it guards that shape rather than reproducing the bug.Test Result
As measured when this PR was opened, with the two Qwen3 tests; current suite numbers are under Test Plan.
The 24 errors were all
tests/parser/test_harmony.py::TestAdjustRequestand were a missing dependency in that test image, not this change. Control on the same tree with the engine reverted:40 passed, 24 errors, the same 24.The red arm reproduces the exact error string seen in production, including the column.
Field evidence
Qwen3.8-27B-NVFP4, temperature 1.0, top_p 0.95,
max_tokens3000, onewrite_filetool withpathandcontentstring properties, prompt asking for a 40-line markdown document. Requests sent directly to the vLLM worker, so no router, gateway or proxy is in the path. 7 of 25 requests produced unparsablearguments:All 7 fail with
Unterminated string. 5 of the 7 carry no stray markup at all: the document just stops, which is what amax_tokenscutoff looks like from the client side. Run 05 ends... confirm the handler module is importable from the worker environment.with no closing quote and no closing brace.Two caveats on that rate, because it is not a field rate.
max_tokens: 3000against a prompt asking for 40+ lines manufactures length stops, so 7 of 25 overstates what a caller with a large budget would see. And the harness only persisted failures, so the 7 files are the failures out of runs indexed 0 to 24, not a sample of all outcomes.Live end-to-end validation (matched patched vs unpatched arms)
Unit tests establish the mechanism; this establishes that the fix removes the defect from a real server.
Host kebab-spark-3, NVIDIA GB10 (sm_121), aarch64. vLLM
0.26.1rc1.dev1164+g794b7b354, editable install of this branch. Modelunsloth/Qwen3.8-27B-NVFP4, snapshota767244d27bd, on local NVMe. Identical server flags in all four runs:The parser actually selected was read from the live interpreter rather than the log:
qwen3_xml -> Qwen3EngineToolParser,_parser_engine_cls = vllm.parser.qwen3.Qwen3Parser,issubclass(Qwen3Parser, ParserEngine) = True,_flush_arg_converterpresent. So the changed code is on the path.Arms were switched by applying and reverting the isolated
parser_engine.pydiff on ONE install (pure Python, no rebuild), restarting the server between arms, and asserting the patch state from the running venv each time viahasattr(Qwen3Parser, "_close_streamed_json").Run A, the sampling parameters from the field capture (temperature 1.0, top_p 0.95,
max_tokens3000):argumentsAll 4 unpatched failures carry the identical signature
Unterminated string starting at: line 1 column 34 (char 33), char 33 being the opening quote of thecontentvalue. The final SSE frame of each is an ordinary mid-word content delta carryingfinish_reason, with no closing flush delta at all:Run A's patched arm is reported for completeness and is not sufficient on its own. None of its 40 requests reached the token cap (max 1251 argument deltas, every content value ended cleanly), so that arm never exercised the trigger. In the unpatched arm 8 of 40 did reach the cap: 4 during reasoning before any tool call (empty
arguments), and 4 inside an open<parameter=>, all 4 of which were malformed. Conditional on the trigger, that is 4 of 4.Run B, trigger forced on every request. Identical to Run A except
max_tokens400 andchat_template_kwargs: {"enable_thinking": false}, applied to BOTH arms. Removing the variable-length reasoning block makes the cap land inside the tool call rather than inside reasoning:argumentsAll 80 requests truncated at the cap (SSE frame counts 341 to 364 against
max_tokens400), so the triggering condition held on every one. All 40 unpatched failures were again exactlyUnterminated string starting at: line 1 column 34 (char 33). All 40 patched responses parse, with shape{"path": "README.md", "content": ...}, content truncated mid-token (1340 / 1495 / 1759 chars for min / median / max) and a final delta of exactly"}.Separately, the 4 malformed
argumentsstrings captured in unpatched Run A were fed to_json_prefix_terminatordirectly: each yields terminator"}and then parses, preservingpathand the truncatedcontent(1247 to 3708 chars).0 malformed in 80 patched requests, against 4 of 40 at the natural rate and 40 of 40 with the trigger forced.
The finish_reason made this harder to notice
On every failure in both runs,
finish_reasonwas"tool_calls"andstop_reasonwasnull- never"length". On that build (g794b7b354),serving.py:761-762setfinish_reasontotool_callswhenever a tool call was streamed, whatever the engine's real stop reason. #46303 (b33c8a657d48, 2026-09-23) narrowed that tooutput.finish_reason == "stop"(serving.py:765-770on main), so amax_tokenscutoff now reportslength. A stop-string cutoff still reportstool_calls, and on main without this PR itsargumentscan still end mid-value with nothing in the response saying so.Model evaluation
None applies, and the reason is worth stating rather than leaving as a silent omission. This change runs after generation, in the parser's end-of-stream flush and in schema coercion of the parsed arguments. It cannot affect sampling, logits or model output, and accuracy benchmarks do not exercise tool-call streaming at all.
The question that does matter is whether it alters output on well-formed tool calls. Measured when this PR was opened: six well-formed Qwen3 tool calls (single parameter, two parameters, embedded quotes and newlines and backslashes, JSON braces inside the value, an empty value, multibyte content), each fed as a whole string, character by character, and at every single split point - 794 stream variants in total - through both the patched and the unpatched engine. The streamed
argumentsare byte-identical between the two arms in all 794 variants.Two honest notes on that number. 84 of the 794 variants produce non-empty arguments, and all 84 are valid JSON; the whole-string and character-by-character feeds produce the complete correct JSON for all six inputs. The remaining variants emit nothing, in both arms alike - a pre-existing interaction between a two-chunk split landing mid-literal and the incremental lexer, which this PR does not touch and does not change.
For the coercion change in
9c69f476174d:json.dumps-built converter output is byte-identical to before (60,000 randomized inputs, above). Well-formed Inkling calls change only where a value is coerced, and there the streamed arguments now equal the non-streaming result.Notes
AI assistance (Claude Code) was used for the code reading, the converter measurements, the tests and this description. The submitter reviewed every changed line and ran the tests above.