[Parser][Bugfix] Ensure tool call or other special tokens don't leak in non-streaming tool parsing - #46875
Conversation
|
Opened as a draft while I do some more testing of this in real-world scenarios. |
…per-model config Special tokens (turn markers, channel delimiters, etc.) were only being filtered via token IDs, so they leaked into response content on the non-streaming text path of engine-based parsers (skip_special_tokens=False). The per-model workaround — hand-maintained drop_tokens sets and adjust_request overrides toggling skip_special_tokens — was fragile and required config for every new model. Replace this with automatic discovery: build __DROP__ terminals from tokenizer.all_special_tokens (minus already-configured terminals) and route them through the existing scanner/lexer/state-machine pipeline. This works for both paths — token IDs (streaming) and text (non-streaming) — with zero per-model configuration. Removes: drop_tokens config field, STRUCTURAL_DROP_TOKENS constant, drop_token_ids filtering in TokenIDScanner, per-model drop token lists (e.g. _GEMMA4_MODEL_DROP_TOKENS), and the Gemma4 adjust_request override. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ben Browning <bbrownin@redhat.com>
e0b7b16 to
4776856
Compare
… tool parsing Separate the two uses of skip_tool_parsing into distinct mechanisms: - skip_tool_parsing (StreamingParserEngine): preserves tool terminal text as content for the reasoning adapter's second-pass tool parser. - _suppress_tool_calls (ParserEngine): lets the state machine parse tool calls normally (consuming terminals via transitions) but filters out all tool events in _events_to_delta, so no tool markup leaks into content and no tool call objects are returned. Previously tool_choice="none" set skip_tool_parsing=True, which emitted tool terminal text as content — the opposite of what callers expect. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ben Browning <bbrownin@redhat.com>
Add non-streaming parse() coverage for tool_choice='none' suppression and auto-drop token stripping, complementing the existing streaming replay tests. Consolidate shared test helpers (DUMMY_TOOLS, parse_non_streaming) into replay_harness.py and replace manual drop-token leakage loops with assert_no_terminal_leakage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ben Browning <bbrownin@redhat.com>
_single_pass_parse processes the entire text in one shot but called _events_to_delta without finished=True, so whitespace-only content was deferred and never flushed. Pass finished=True to match streaming parity and add a consistency test covering the fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ben Browning <bbrownin@redhat.com>
…ant special tokens The auto-drop mechanism from 4776856 silently drops special tokens not in configured_texts, including gemma4's <|"|> string delimiter. Without it, _parse_gemma4_args treats commas inside string values as field separators (BFCL multi_turn_base 81.50% → 69.50%). Add preserve_tokens to ParserEngineConfig so models can exempt tokens from auto-drop without making them state-machine terminals. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ben Browning <bbrownin@redhat.com>
|
This took some time to clean up and handle all special cases, but this hardens our special token handling in the new parser engine by ensuring none ever leak by default, emulating what That also exposed a case where we were sometimes leaking tool calls out for Gemma4 today only when tool_choice="none", enable_thinking=False, and tools provided in a request. That was closely tied to the special token dropping code paths, and cleaning that up exposed this a bit wider, so this fixes that as well by ensuring we don't returned parsed tool calls when tool_choice="none" but also don't return the unparsed tool call content. |
|
@claude review |
There was a problem hiding this comment.
Architectural changes to the parser engine (auto-drop via tokenizer discovery, new tool-call suppression path, removal of per-model drop_token lists) — this needs a human reviewer with parser-engine context to sign off.
Extended reasoning...
Overview
This PR refactors how special tokens are filtered in the new parser engine across 14 files (8 in vllm/parser/ core code + 6 test files). The main changes:
- Replaces hand-maintained
drop_tokenssets with auto-discovery viatokenizer.all_special_tokens, building__DROP__terminals routed through the existing lexer/state-machine pipeline. - Introduces
preserve_tokensinParserEngineConfigfor tokens that need to bypass auto-drop (e.g. Gemma4's<|"|>string delimiter). - Separates
tool_choice="none"handling fromskip_tool_parsinginto a new_suppress_tool_callsmechanism that consumes tool terminals via state-machine transitions but filters out tool events. - Removes Gemma4's
adjust_requestoverride and the hardcoded_GEMMA4_MODEL_DROP_TOKENSset. - Flushes deferred whitespace at end-of-stream in
_single_pass_parse. - Simplifies
TokenIDScannerby removing the now-redundantdrop_token_idsparameter.
Security risks
No direct security risks. The changes do not touch auth, crypto, permissions, or input validation at trust boundaries. They affect parsing of model-generated output, where the worst-case failure mode is malformed tool calls or leaked delimiter text into response content — a correctness issue, not a security one.
Level of scrutiny
High. This touches the core parser engine that handles tool calling and reasoning extraction for all engine-based parsers (Gemma4, Qwen3, Hermes, etc.). Tool calling is a primary feature; a regression here breaks downstream agentic workflows. The changes are not mechanical — they shift architectural responsibility (special-token filtering moves from per-model config into the engine's StreamingParserEngine), which deserves a maintainer who understands the lexer/state-machine layering and the implications across all parsers in registered_adapters.
Other factors
- Test coverage is substantial: new replay tests across all registered parsers verify drop tokens are stripped in both streaming and non-streaming, regression tests for tool_choice="none", and the bug-hunting system found no issues.
- The author included live-model testing against Gemma4 (12/12 manual tests passing post-fix, BFCL multi_turn_base within noise of pre-PR baseline).
- One reviewer (sfeng33) explicitly requested my review, but no human approval has been recorded yet.
- The Gemma4
adjust_requestremoval is load-bearing: the new testtest_gemma4_keeps_skip_special_tokens_false_when_nothing_to_preserveinverts the prior expectation (wasTrue, nowFalse). That inversion relies entirely on the auto-drop mechanism working for every special token the tokenizer reports — a reviewer should confirm the assumption holds across the model set vLLM ships.
|
Merging latest main, the new tests added here catch some bugs in the Kimi parser implementation that was merged since I opened this. Debugging locally and will add another commit to fix that in a bit. |
The test expectation function used TOOL_START (<|tool_call_begin|>) as the content boundary for all parsers, but Kimi K2 wraps individual tool calls in a section with TOOL_SECTION_START (<|tool_calls_section_begin|>) which is the actual CONTENT→tool state transition. Use that terminal as the boundary when present. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ben Browning <bbrownin@redhat.com>
|
The Kimi parser was fine actually but the test expectations in the replay suite around the new drop tokens and suppression of tool calls when needed had to be tweaked a bit to handle the Kimi parser's slightly different tool call sections. |
…in non-streaming tool parsing (vllm-project#46875) Signed-off-by: Ben Browning <bbrownin@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…endored PRs, drop #46225 Base cu129-nightly a65f93fb -> 93d8f834 (2026-07-01). The new base carries merged vllm-project/vllm#46875 ("special tokens don't leak in tool parsing"), which reworks the streaming parser engine + token-id scanner. That supersedes our vendored vllm-project/vllm#46225 (non-streaming special-token strip) -> dropped (patch + Dockerfile block + tripwire removed). Re-vendored the remaining open PRs rebased onto the new base: - vllm-project/vllm#45877 (DeepSeek V4/V3.2 engine parsers) -- rebased; ParserEngineConfig kwargs verified against the post-#46875 dataclass (all fields present). - vllm-project/vllm#46995 (DSpark spec decode) -- refreshed to the PR's 2026-06-30 head, vllm/-only (20 files; tests/ excluded). - vllm-project/vllm#46257 (deepseek_v4 add_generation_prompt honor) -- rebased (patch unchanged). All patches are vllm/-only and validated with `patch -p1` against a vllm/-only tree -- matching the build: the runtime image has no git, so it applies with `patch -p1` into site-packages, where tests/ does not exist (a tests/ hunk is what broke the first build). CI build (tripwires) + deploy validate. Post-deploy: re-run the streaming tool-arg leak repro (target of #46875) and the non-streaming BOS-leak repro (previously covered by #46225).
…l-token-leak fix) (#26) feat: bump base to nightly 93d8f834 (brings merged #46875), refresh vendored PRs, drop #46225 Base cu129-nightly a65f93fb -> 93d8f834 (2026-07-01). The new base carries merged vllm-project/vllm#46875 ("special tokens don't leak in tool parsing"), which reworks the streaming parser engine + token-id scanner. That supersedes our vendored vllm-project/vllm#46225 (non-streaming special-token strip) -> dropped (patch + Dockerfile block + tripwire removed). Re-vendored the remaining open PRs rebased onto the new base: - vllm-project/vllm#45877 (DeepSeek V4/V3.2 engine parsers) -- rebased; ParserEngineConfig kwargs verified against the post-#46875 dataclass (all fields present). - vllm-project/vllm#46995 (DSpark spec decode) -- refreshed to the PR's 2026-06-30 head, vllm/-only (20 files; tests/ excluded). - vllm-project/vllm#46257 (deepseek_v4 add_generation_prompt honor) -- rebased (patch unchanged). All patches are vllm/-only and validated with `patch -p1` against a vllm/-only tree -- matching the build: the runtime image has no git, so it applies with `patch -p1` into site-packages, where tests/ does not exist (a tests/ hunk is what broke the first build). CI build (tripwires) + deploy validate. Post-deploy: re-run the streaming tool-arg leak repro (target of #46875) and the non-streaming BOS-leak repro (previously covered by #46225).
…in non-streaming tool parsing (vllm-project#46875) Signed-off-by: Ben Browning <bbrownin@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…in non-streaming tool parsing (vllm-project#46875) Signed-off-by: Ben Browning <bbrownin@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…in non-streaming tool parsing (vllm-project#46875) Signed-off-by: Ben Browning <bbrownin@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: wang.yuqi <yuqi.wang@daocloud.io>
…in non-streaming tool parsing (vllm-project#46875) Signed-off-by: Ben Browning <bbrownin@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…in non-streaming tool parsing (vllm-project#46875) Signed-off-by: Ben Browning <bbrownin@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…in non-streaming tool parsing (vllm-project#46875) Signed-off-by: Ben Browning <bbrownin@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…in non-streaming tool parsing (vllm-project#46875) Signed-off-by: Ben Browning <bbrownin@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: root <root@smci355-ccs-aus-m02-09.cs-aus.dcgpu>
Adapt the merged parser-engine prerequisites from vLLM vllm-project#46344 and vllm-project#46875 onto the MoET v0.24 lineage so the DeepSeek V4 parser-engine migration has its history accounting, special-token handling, and replay harness support. Signed-off-by: John Pezzulli <38448408+jpezzulli@users.noreply.github.com>
Purpose
In the new parser engine, non-streaming tool parsing had several paths where tokens that should be consumed or suppressed leaked into response content.
Auto-drop special tokens
Special tokens (turn markers, channel delimiters, etc.) were only being filtered via token IDs, so they leaked into response content on the non-streaming text path of engine-based parsers (
skip_special_tokens=False). The per-model workaround — hand-maintained drop_tokens sets andadjust_requestoverrides togglingskip_special_tokens— was fragile and required config for every new model.Replace this with automatic discovery: build
__DROP__terminals fromtokenizer.all_special_tokens(minus already-configured terminals) and route them through the existing scanner/lexer/state-machine pipeline. This works for both paths — token IDs (streaming) and text (non-streaming) — with zero per-model configuration.This effectively emulates
skip_special_tokens=Truebehavior, but after we've handled all the special tokens needed by the parsing engine. We cannot directly setskip_special_tokens=Trueinadjust_requestbecause then the reasoning and tool parsers would never see those special tokens they need to do their parsing work.Some models (currently only Gemma 4 for models using the parser engine) need some special tokens to be preserved outside of the lower layers of the parser and exposed to the upper levels, to be used during parsing of tool call args. There's a new
preserve_tokensin theParserEngineConfigthat lets a parser configure tokens to not automatically drop.Suppress tool calls for tool_choice=none
Once we fixed the above, it exposed a bug in our
tool_choice="none"and thinking disabled handling in the new parser engine. Previouslytool_choice="none"setskip_tool_parsing=True, which emitted tool terminal text as content — the opposite of what callers expect. Separate this into a distinct_suppress_tool_callsmechanism that lets the state machine parse tool calls normally (consuming terminals via transitions) but filters out all tool events, so no tool markup leaks into content and no tool call objects are returned. This was partially masked for some models (like Gemma4) because they would overrideadjust_requestand setskip_special_tokens=Truein this case before, where now they rely entirely on the parser engine to strip these special tokens.There was inconsistency here in the streaming vs non-streaming paths, so Gemma4 only had this exposed on main in the streaming path but after removing its adjust_request override it exposed this in both streaming and non-streaming.
Flush deferred whitespace in non-streaming parse
Fixing that exposed one last bug, where non-streaming scenarios that end up with whitespace-only content was deferred but never flushed within the engine. This is a small consistency fix at the engine level between streaming and non-streaming that I don't believe was actually visible from the external serving layer.
Test Plan
Unit Tests
Multiple new unit tests were added, using TDD to ensure the unit tests triggered the errors properly before the fix and that they passed after the fix.
Manual test against live models
This uses the simple test script from https://gist.github.com/bbrowning/490740a32acebc262cc06b4d8c426955 saved as
test_drop_token_leaks.py.Run a model, such as Gemma 4 31B:
And run the manual test suite that checks various combinations of tool_choice and thinking enabled/disabled in streaming and non-streaming to detect any leaked special tokens:
BFCL testing with Gemma4
Gemma4 was the one that had the most complete tests and expectations around adjust_request, skip_special_tokens conditionally set, and dropping of tokens. And, it has a special case where some of its special tokens (string delimiter) has to survive all the way out to our parser engine arg converter layer to be properly handled. So, I centered real-world testing on this model.
Test Result
Unit Tests
Manual test against live models
Before, exposing tool_choice="none" and thinking explicitly disabled bug:
After, with this PR applied to the running server:
BFCL testing with Gemma4
Before
After
I see this fluctuate up and down by as much as 1-2% every run, so within 0.5% is considered identical for this model and this eval.