fix(server): keep assistant text when a delta also carries tool calls

[janhilgard]

Fixes the streaming half of what @Thump604 found on #676: "buffered streaming drops text before a tool call when the text and complete DSML block arrive in one delta; the parser returns only tool_calls."

The parser side lands in #676. This is the server side, and it is not DeepSeek-specific — any parser that buffers across a block hits it whenever the block arrives whole.

The bug

Every streaming path reads tool_calls in result as "suppress everything". A parser that has buffered prose and then sees the entire tool-call block in one delta has nowhere else to put that prose: the block is complete, so there is no later delta to flush into, and the non-streaming path returns the same text without complaint. The assistant message loses text based on nothing but how the model's output happened to be chunked.

Three call sites, three spellings of the same assumption:

where	what it did
_parse_streaming_tool_content (Anthropic ×2)	suppress = "tool_calls" in result, and both callers continue on it — despite already doing tool_result.get("content", "") on the next line
Responses path	if "tool_calls" in tool_result: continue before reading content at all
OpenAI path	emitted the tool-call chunk, then continue — dropping content in the same result

The fix

All three emit the text. The OpenAI path sends it as its own chunk ahead of the tool-call chunk, which is the shape the API expects; the other two fall through to content handling they already had.

Suppression is unchanged when there is nothing to show: tool calls alone, empty content alongside calls, and a None result meaning "inside markup, keep holding".

Verification

tests/test_streaming_content_with_tool_calls.py covers all five result shapes plus that the accumulated text still advances. Mutation-checked: restoring suppress = "tool_calls" in result fails the text-alongside-calls test.

Repo suite: 2300 passed, with the 498 tool-parser tests unaffected — that mattered here, since this touches shared code behind all 19 parsers. The four failures I see locally reproduce on pristine main (three Python 3.14, one missing ffmpeg).

The end-to-end regression through a parser that actually produces both in one delta goes with the parser fix in #676, so it does not sit here permanently skipped.

[Thump604]

Good catch and the direction is right, but the tests do not cover the server paths changed here. They only call _parse_streaming_tool_content, so the Responses path and both OpenAI branches can regress while this stays green. Please add real streaming regressions for Responses, Anthropic, and OpenAI with and without reasoning. Assert text exactly once, tool calls exactly once, text before the call, and the terminal finish/usage event. This shared path needs integration coverage in the same PR.

[janhilgard]

Fair, and the gap was exactly as you describe: I changed three call sites and tested one helper. Added tests/test_streaming_content_with_tool_calls_integration.py, which drives the real streaming generators — stream_chat_completion, _stream_anthropic_messages and _stream_responses_request — with a parser that returns content and tool calls in one delta.

Assertions per path: text exactly once, tool calls exactly once, text before the call, and the terminal event (finish_reason + [DONE] for OpenAI, message_stop for Anthropic, response.completed for Responses). Plus the OpenAI path with a reasoning parser active, since that reaches the same tool branch by a different route.

Each path is mutation-checked independently, which is the part that matters given your point:

reverted	caught by
Anthropic helper back to suppress = "tool_calls" in result	Anthropic integration test + the unit test
OpenAI leading-content emit removed	both OpenAI tests
Responses continue before reading content	Responses test

So none of the three can regress silently now.

Two things worth noting from writing them, since both would have made the tests lie:

• The streaming paths only consult the parser when the delta looks like it might contain tool markup (_streaming_tool_markup_possible). My first version streamed plain text, so the parser was never called and the tests passed against the broken code. The fake output now trips that gate deliberately.
• The Responses protocol repeats the final text in its done/completed events by design — six occurrences in the body is correct, not duplication. That test counts response.output_text.delta events instead of substring hits in the whole stream.

Repo suite: 2304 passed, lint and black clean under the exact CI invocation.

[Thump604]

Better coverage, but one real path is still broken. With a Responses reasoning parser active, _stream_responses_request emits delta_msg.content and immediately continues, so the tool parser is never called. I reproduced the tool marker leaking as output text with zero function-call events and a normal response.completed. Please route reasoning content through the tool parser and add that integration case. Also tighten the committed assertions to match the stated coverage: Anthropic and Responses should assert the call once and after the text; reasoning OpenAI should assert the call and terminal event too. Keeping changes requested.

[janhilgard]

You found a worse bug than the one this PR started from, and my tests could not have caught it — the Responses reasoning case was the one combination I did not cover.

Confirmed: _stream_responses_request emitted delta_msg.content and continued, so with a reasoning parser active the tool parser was never consulted at all. Tool markup left the reasoning parser as ordinary text and went out as visible output. That is not a dropped-text bug, it is raw markup reaching the client.

Fixed by routing the reasoning content through the same gate the non-reasoning branch below already uses — the markup check, _extract_streaming_tool_delta, and _TOOL_MARKUP_PATTERN cleanup. New integration test asserts the marker never appears in any response.output_text.delta, that the text arrives exactly once, and that one function_call item is announced. Mutation-checked: bypassing the parser fails it with '<tool_call>' not in '<tool_call>'.

Assertions tightened as asked, and two of them needed a different shape than I first wrote, which is worth recording:

• Anthropic and Responses do not emit tool calls from the streaming deltas at all — both re-parse the accumulated text once at the end via _parse_tool_calls_with_parser. My stub only implemented extract_tool_calls_streaming, so those paths looked like they emitted zero calls and the new assertion failed for the wrong reason. The stub now answers the terminal re-parse too, as a real parser does.
• On Responses, counting substring hits does not work for the call either: added/done/completed repeat it by design, three occurrences being correct. That assertion counts response.output_item.added events with a function_call item instead.

Reasoning-OpenAI now asserts the call once and the terminal finish_reason alongside the text, as requested.

Five integration cases, each mutation-checked against its own path. Repo suite: 2305 passed, lint and black clean under the exact CI invocation.

[Thump604]

Re-reviewed the updated head. The Responses reasoning path now routes content through the tool parser, and the committed integration coverage exercises OpenAI (with and without reasoning), Anthropic, and Responses with text/call ordering, exactly-once assertions, and terminal events. Focused local run: 11 passed. CI is green. No remaining blocker.

[waybarrios]

Nice direction, but content is not always text from before the call. The real Poolside streaming parser keeps parsing after </tool_call> and puts text from both sides into the same field.

For Before<tool_call>write_file</tool_call>After, it returns content="BeforeAfter" plus the call. This branch then emits BeforeAfter before the call, while the non-streaming parser returns only Before. That leaks and reorders After only in streaming responses.

Could we either make Poolside return only pre-call content when calls are present, matching non-streaming behavior, or extend the parser result with ordered segments? Please also add an endpoint regression using the real PoolsideV1ToolParser for this input and assert that the reconstructed text matches non-streaming output and the call appears once.

[waybarrios]

These tests currently are not executed by CI. Every pytest command in the workflow uses an explicit file list, and neither new file is included. Direct collection finds 11 tests, while the required jobs collect none of them.

There is a small trap if we add this file to the existing Apple command: -k "not Integration" matches the filename and deselects all five endpoint tests. A separate Apple Silicon invocation would be the smallest fix:

pytest \
  tests/test_streaming_content_with_tool_calls.py \
  tests/test_streaming_content_with_tool_calls_integration.py \
  -v --tb=short

[janhilgard]

Heads up: this one conflicts after #679/#684 landed, and it needs more than a rebase — I have the resolution locally but I am not pushing it, because it regresses.

The single server.py conflict is benign: main now guards the Responses path with

if "tool_calls" in tool_result:
    content = tool_result.get("content", "")
    if not content:
        continue

which is equivalent to this branch's formulation, so I took yours and the diff there is zero.

The problem is elsewhere. Since this PR was opened, main gained a finalize-flush:

if tool_result is None and output.finished:
    tool_result = _finalize_streaming_tool_result(tool_parser, tool_accumulated_text)

This branch routes reasoning-branch content through the tool parser and, on the "no markup possible" path, appends it to tool_accumulated_text and emits it. With the new finalize flush, that buffer goes out a second time at end of stream. Two tests catch it:

test_text_precedes_the_call_and_each_appears_once
  -> text emitted 2 times: ['Checking the weather.', 'Checking the weather.']
test_llama_formats_stream_through_server_gate[content-and-python-tag]
  -> assert 'Before  AfterBefore  After' == 'Before  After'

So the two changes overlap semantically rather than textually, and reconciling them is a design question about who owns the buffer: either the reasoning branch stops accumulating what it has already emitted, or finalize learns what was already sent. Both are plausible, and both are easy to get subtly wrong in a streaming path whose failure mode is silently duplicated or dropped user-visible text.

Do you have a preference? If the finalize path is meant to be the single owner of end-of-stream flushing, I will restructure this branch around that and re-push. I would rather ask than hand you a regression that shows up on one parser family only.

[Thump604]

Blocking correctness issue remains on the current head, independent of the stale conflict:

The current PoolsideV1ToolParser.extract_tool_calls() returns non-streaming content only from before <tool_call> (vllm_mlx/tool_parsers/poolside_v1_tool_parser.py:126-134). Its streaming state machine consumes the closing tag and then runs _consume_text_before_tool() again (:200-210 and :303-310), so a single delta such as Before<tool_call>... </tool_call>After can return content="BeforeAfter" alongside the call. The PR's OpenAI branch emits that content before the call, while the non-streaming path returns only Before; the trailing text is reordered and the two API modes disagree. The added tests use a canned parser result and do not exercise the real Poolside parser or this post-call case.

Please rebase on current main and make the contract explicit: either discard post-call text consistently or preserve it as ordered segments in both streaming and non-streaming responses. Add a real Poolside endpoint regression covering text before/inside/after the call, exactly-once ordering, reasoning and non-reasoning paths, and terminal events. Also resolve the current-main finalization interaction (_finalize_streaming_tool_result) as a buffer-ownership decision rather than a textual conflict resolution.

The new tests are not currently in CI: .github/workflows/ci.yml uses explicit pytest file lists and omits both tests/test_streaming_content_with_tool_calls.py and tests/test_streaming_content_with_tool_calls_integration.py; the Apple command also filters out Integration. Add a CI invocation that actually collects them, then rerun the current checks and resolve the two outstanding inline discussions. My earlier approval was for the old head and is not evidence for this conflicting state.

[janhilgard]

Rebased onto current main and pushed. This resolves the conflict I flagged on 19 Aug, and I can now say concretely what the "it regresses" problem was and why it is gone.

What the conflict actually was. Between my branch and main, the streaming tool path grew _requires_eager_tool_streaming() / _finalize_streaming_tool_result() and, in stream_chat_completion(), started carrying parser content in the same chunk as the tool calls:

content=tool_result.get("content") or None,

My branch emits that text as its own chunk before the tool-call chunk. Git merged both cleanly — no conflict marker — so the naive rebase emitted the same text twice. That is what I was seeing and did not want to push blind.

Resolution. Text is emitted once, ahead of the call, and the tool-call chunk no longer repeats it:

tool_calls=tool_result["tool_calls"],
# `leading` above already emitted this text as its own chunk,
# so repeating it here would double it.
content=None,

I also took main's version of the two hunks that genuinely conflicted, because main had independently arrived at the same fix in the non-reasoning branch, just spelled differently:

# main, equivalent to what this PR did:
if "tool_calls" in tool_result:
    content = tool_result.get("content", "")
    if not content:
        continue

So the surviving change is smaller than before: the Responses reasoning branch (which still had if tool_result is None or "tool_calls" in tool_result: content = ""), the shared _parse_streaming_tool_content() suppression predicate, and ordered emission in the two stream_chat_completion() sites. server.py is now +42/-4 rather than +72/-6.

Verification.

Against clean upstream/main with only the test files applied — three of the tests fail, which is the bug still being present:

FAILED test_streaming_content_with_tool_calls.py::TestSuppressionKeepsText::test_text_alongside_tool_calls_is_not_suppressed
FAILED test_..._integration.py::TestOpenAIStream::test_text_precedes_the_call_and_each_appears_once
FAILED test_..._integration.py::TestResponsesStream::test_reasoning_content_is_routed_through_the_tool_parser
3 failed, 8 passed

With the fix:

11 passed

Wider regression run over the streaming, tool, responses and chat suites:

1050 passed, 4 skipped, 1962 deselected
tests/test_server.py: 144 passed, 3 deselected

black clean.

One caveat I want to be straight about: this is unit-level verification only. I could not run it against a live server for this revision — the box is memory-committed to another workload — so the ordering guarantee (text chunk strictly before the tool-call chunk) is asserted by the integration test's chunk-index check rather than observed on a real stream.

@waybarrios this is ready for another look whenever you have time — the CHANGES_REQUESTED from the earlier round is what is holding it now.

[Thump604]

Thanks, Jan. I rechecked 8d51a3a. The separate text chunk plus content=None in the tool-call chunk addresses the duplicate-emission conflict you described.

Two existing review items still need closure:

1. Poolside text ordering/parity. The parser still combines text from both sides of a call: Before<tool_call>write_file</tool_call>After becomes content="BeforeAfter", while non-streaming returns only Before. The updated OpenAI branch sends that combined text before the call. This parser mismatch already exists on main; it is not a new defect introduced by your rebase. The new endpoint tests still use the canned parser, so they do not settle Wayner's real-Poolside case. Matching the existing non-streaming prefix-only behavior would be the narrower option; an ordered-segment redesign is not required. A separately linked prerequisite is also fine if that parser correction belongs outside this PR.
2. CI execution. Neither new test file is in the current workflow's explicit pytest lists. The ten green checks therefore do not execute these eleven regressions. The existing CI thread has the small fix: a separate Apple Silicon invocation of both files, without -k "not Integration", which would exclude the endpoint file by name.

This follow-up is based on source and CI-configuration inspection; I have not run a new live-server test. I am not asking you to repeat the broad suite or produce another model benchmark. My remaining requests are the real-parser regression/contract disposition and wiring those tests into CI.

[janhilgard]

Both items addressed in 4531162. Also rebased onto current main, which had moved six commits ahead.

1. Poolside prefix-only parity

You were right that this was the substantive one, and my first attempt at it was wrong in an instructive way, so here is what landed and what did not.

What I tried first: recompute the text from the raw accumulated deltas, cutting at the earliest tool marker. That reproduces the non-streaming rule directly, and it passed the new parity test — but it broke test_deepseek_v4_split_marker_never_leaks:

assert 'Checking.\n\nChecking.\n\n<' == 'Checking.'

Two defects in one: the prefix covers text the client has already received as ordinary content deltas, so it gets re-sent; and on a marker split across deltas the cut lands mid-marker and leaks the <. I checked that test on clean upstream/main first — it passes there, so this was my regression, not a pre-existing one.

What landed instead keeps the parser's per-delta content (which is by construction the not-yet-emitted text) and removes only the trailing part, and only when it matches the raw text following the call:

def _text_after_tool_call(parser, accumulated_text: str) -> str:
    end_marker = getattr(parser, "_END", None)
    if not isinstance(end_marker, str) or not end_marker:
        return ""
    tail = accumulated_text.rfind(end_marker)
    return "" if tail < 0 else accumulated_text[tail + len(end_marker) :]

Parsers that do not declare _END are untouched, and the strip is a no-op unless the content actually ends with the post-call text. This is the narrower option you suggested rather than an ordered-segment redesign.

2. Real-parser regression, not the canned one

TestRealPoolsideParserParity drives the actual PoolsideV1ToolParser through stream_chat_completion with Before<tool_call>write_file…</tool_call>After, and asserts the streamed text equals what extract_tool_calls returns for the same output — so the two paths are pinned to each other rather than to a number I typed in.

Mutation-checked, because a regression test that cannot fail is not one. With the strip removed:

AssertionError: streamed ['BeforeAfter'], non-streaming returned 'Before'

which is your Before…After case verbatim. With it restored, 2 passed.

There is also a sibling test asserting the non-streaming contract straight from the parser, so if that behaviour ever changes deliberately, the failure says which side moved.

One fixture correction fell out of this. The canned tests fed <tool_call> alone as the model output while the parser returned "Checking the weather." — text that appears nowhere in that output. A real buffering parser collected that prose from earlier deltas, so it is in the accumulated text. The fixture now emits TEXT + MARKER. Same assertions, but the input is no longer impossible.

3. CI wiring

Added a separate Apple Silicon step, without -k "not Integration" for exactly the reason you gave:

      - name: Run streaming text-with-tool-calls regression tests
        run: |
          # Kept out of the list above because that invocation filters with
          # -k "not Integration", which would deselect the endpoint file by
          # name and silently skip every regression in it.
          pytest \
            tests/test_streaming_content_with_tool_calls.py \
            tests/test_streaming_content_with_tool_calls_integration.py \
            -v --tb=short \
            -m "not slow"

Ran that exact invocation locally: 13 passed.

Verification

tests/test_server.py                                     169 passed
streaming / tool / responses / chat suites              1074 passed, 4 skipped
new files (as CI invokes them)                            13 passed

black clean. Two of my own test stubs needed reset_state(self, *args, **kwargs) after the rebase — main now passes implicit_mode.

As before, this is unit-level only; I have not run a live server for this revision.

[Thump604]

Thanks, Jan. I checked 4531162 and both Apple jobs in run 33884451655: all 13 tests actually ran and passed. The CI-wiring request is satisfied. Keeping the parser's incremental output also addresses the duplicate-prefix/split-marker problem you demonstrated; I am not asking you to return to raw-prefix recomputation.

One part of the existing Poolside parity request remains: the new real-parser endpoint test supplies the whole response in one delta. With the same payload split as:

1. Before<tool_call>write_file<arg_key>path</arg_key><arg_value>/tmp/a.py</arg_value></tool_call>
2. After
3. The existing empty finished delta

the source path still emits the second delta as ordinary content: Poolside returns a content-only result, so it bypasses the helper inside the tool_calls branch and reaches the normal content emission path. That gives streamed BeforeAfter versus non-streaming Before. This is a source trace, not a locally executed reproduction, and it is the pre-existing chunk-dependent parity gap, not a new regression attributed to your helper.

Could you parameterize the existing real-Poolside endpoint test with that split and preserve its prefix-only contract across deltas, while retaining the DeepSeek split-marker regression? That is the only remaining request from me. No new benchmark or parser redesign; the separately linked Poolside prerequisite remains an option as discussed.

[janhilgard]

Good catch — that gap is real and my helper could not have covered it, because it lives inside the tool_calls branch and the trailing delta never reaches that branch.

What was wrong

Your trace is right. With the payload split so that the call completes in delta 1 and After arrives in delta 2, Poolside returns a content-only result for delta 2, so it falls through to the normal content emission path and goes out verbatim. Streamed BeforeAfter, non-streaming Before.

The fix

Suppress ordinary content once a call has already been emitted, for parsers whose non-streaming contract keeps only the prefix:

# Normal content from tool parser
content = tool_result.get("content", "")
if (
    content
    and tool_calls_detected
    and _parser_drops_text_after_tool_call(tool_parser)
):
    content = ""

with the capability scoped rather than applied globally:

def _parser_drops_text_after_tool_call(parser) -> bool:
    """Whether this parser's non-streaming contract discards post-call text."""
    return isinstance(getattr(parser, "_END", None), str)

I checked which parsers that selects before writing it — _END as a class attribute is declared only by Glm47ToolParser/PoolsideV1ToolParser. The DeepSeek-V4 parser uses module-level constants (TOOL_CALLS_END, INVOKE_END), so it is untouched and its split-marker regression keeps passing. That was deliberate: the last time I reached for a general rule here it broke exactly that test.

Test

Parameterised as you asked, three shapes over the same payload:

• single-delta — the original
• text-after-call-in-a-later-delta — your case
• prefix-call-and-suffix-each-in-their-own-delta — prefix, call and suffix each separate

All three assert against extract_tool_calls() on the same output rather than a literal, so the two paths stay pinned to each other.

Mutation-checked. With the suppression disabled, the two new shapes fail and the original still passes:

FAILED …[text-after-call-in-a-later-delta]           - Before  + BeforeAfter
FAILED …[prefix-call-and-suffix-each-in-their-own-delta]
2 failed, 2 passed

so they cover the delta-boundary behaviour specifically, not just the end state.

Verification

streaming files + test_server.py + poolside parser      196 passed
streaming / tool / responses / chat suites             1076 passed, 4 skipped
after rebase onto current main                          184 passed

black --check and the exact CI ruff invocation both clean — I run those two locally now rather than black alone, after sending an F541 to the runner on #767 yesterday.

Rebased onto current main; server.py is +116/-4.

[Thump604]

Rechecked exact head b34bf8c. The new real-Poolside parameterization covers the original one-delta case plus both requested post-call multi-delta shapes, and it binds streamed text to the parser’s non-streaming prefix-only result. The suppression remains parser-scoped, preserving the existing DeepSeek split-marker behavior. I also inspected Apple run 33904155771: all 15 mapped regression cases executed and passed with a normal pytest summary. This closes my remaining source and CI requests. I did not require another live model run for this parser/transport correction.

[waybarrios]

I addressed both streaming regressions found during review. OpenAI preserves whitespace when text accompanies tool calls. Responses consistently applies Poolside’s text-before-tool-call policy, with and without reasoning, including text arriving in later chunks.

Added 12 regression cases using the real Poolside parsers. All reproduced the failures before the fix and now pass. Local validation passed 296 parser-related tests and 60 transport tests with simulated engines.