Skip to content

[Bugfix][Frontend] Backport tool truncation finish reasons and cover non-streaming calls - #767

Closed
logprobz wants to merge 1 commit into
local-inference-lab:dev/jovian-judgementfrom
logprobz:fix/jj-tool-finish-reason
Closed

logprobz wants to merge 1 commit into
local-inference-lab:dev/jovian-judgementfrom
logprobz:fix/jj-tool-finish-reason

Conversation

@logprobz

@logprobz logprobz commented Sep 16, 2026 •

Copy link
Copy Markdown

Purpose

Backport the streaming tool-truncation finish-reason correction to
dev/jovian-judgement and fix the additional non-streaming automatic-tool case.
When generated tool output reaches its token limit, the response retains
finish_reason="length" instead of reporting a completed tool_calls response.
This also applies when a complete call was emitted before the engine hit its
limit. Successful automatic/required calls, named calls, ordinary text and the
existing non-streaming missing-reason fallbacks keep their current behavior.

Provenance and duplicate check

  • The streaming condition is the change proposed by Ting Sun in
    vllm-project/vllm#46303,
    also present in #47963.
    Both remain open against upstream main; this is an attributed Jovian
    backport, not another competing upstream submission.
  • Neither covers the non-streaming auto_tools_called branch, which also
    overwrites length when the parser returns a call. The regression uses the
    actual DeepSeek V4.1 parser to reproduce it.
  • Upstream #53256 covers a
    different case: required-tool responses with no parsed call. Its behavior
    is outside this patch. Jovian Fix Codex tool-call poisoning and fatal KV completion races #701 repairs argument finalization and replay,
    not the finish-reason override.
  • Open and closed PR searches and the current target source were rechecked
    immediately before publication. No equivalent Jovian PR was found.

Test Plan

CPU tests use the complete target Python source at
200b6e23590dfe9bab7adf740cdd9facb979a4ed, with this two-file change. The test
container supplies pinned native dependencies without GPU devices. Python 3.12,
Torch 2.13.0, Transformers 5.15.0, tokenizers 0.22.2 and pytest 8.4.1 were used.
The test reuses the suite GPT-2 tokenizer, preserves UTF-8 chunk boundaries,
and executes both response generators with the actual parser.

python -m pytest --confcutdir=tests \
  tests/entrypoints/openai/chat_completion/test_serving_chat.py \
  -k 'tool_call_preserves_engine_finish_reason or streaming_n_gt1_independent_tool_parsers or tool_choice_validation_without_parser or metrics or prompt_tokens_details or streaming_reasoning_usage' -q
pre-commit run --files \
  vllm/entrypoints/openai/chat_completion/serving.py \
  tests/entrypoints/openai/chat_completion/test_serving_chat.py
pre-commit run mypy-3.12 --hook-stage manual --files \
  vllm/entrypoints/openai/chat_completion/serving.py \
  tests/entrypoints/openai/chat_completion/test_serving_chat.py

Test Result

  • Before the implementation fix: 6 failed, 22 passed in the new
    regression matrix. Four failures were streaming automatic/required calls;
    two were non-streaming automatic calls. All six asserted the wrong terminal
    finish reason, not setup or parser errors.
  • After the fix: 36 passed, 0 skipped, including all 28 regression cases
    and eight existing serving controls.
  • All applicable changed-file hooks passed, including Ruff, typo checks,
    Python 3.10 typing and source checks. The Python 3.12 typing hook and Python
    compilation passed separately.

This is a response-reporting correction. Model generation, sampling and
argument-repair logic are unchanged. No model inference evaluation was run for
this PR, and production GLM was not restarted or modified.

AI assistance: Codex helped prepare, review and test this change.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected chat completion finish reasons for tool-calling responses.
    • Truncated or interrupted generations now retain the length finish reason instead of being reported as tool_calls.
    • Completed tool calls continue to report tool_calls when appropriate.
    • Responses without an engine-provided finish reason now default to stop.
    • Applied consistent behavior across streaming and non-streaming responses and supported tool-choice modes.

Backport the streaming stop guard proposed by Ting Sun in
vllm-project#46303 (af50313).
The equivalent streaming change is also proposed in upstream vllm-project#47963.

Preserve length in the non-streaming automatic-tool path as well, and
retain existing missing-reason fallbacks. Add real-parser regressions
for partial and complete calls, normal stops, named tools and text.

Co-authored-by: Ting Sun <suntcrick@gmail.com>
Assisted-by: Codex
Signed-off-by: logprobz <321553542+logprobz@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 25df5e9b-ed4e-4aa4-8584-89fa5c1843fe

📥 Commits

Reviewing files that changed from the base of the PR and between 200b6e2 and 6a7165b.

📒 Files selected for processing (2)
  • tests/entrypoints/openai/chat_completion/test_serving_chat.py
  • vllm/entrypoints/openai/chat_completion/serving.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Streaming and non-streaming chat completion paths now preserve truncation and other engine finish reasons instead of reporting tool_calls. Parameterized tests cover parser behavior across tool choices, streaming modes, and finish-reason values.

Changes

Tool-call finish reason handling

Layer / File(s) Summary
Finish-reason gating and validation
vllm/entrypoints/openai/chat_completion/serving.py, tests/entrypoints/openai/chat_completion/test_serving_chat.py
The streaming path reports tool_calls only for streamed tools with a stop engine finish reason. The non-streaming auto-tool path accepts only unset or stop engine finish reasons. Parameterized tests cover truncation, normal completion, missing reasons, streaming, non-streaming, and tool-choice variants.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: chaunceyjiang

Merge Risk: ⚪ Minimal · up to 6a716

The finish-reason handling change is ready to merge with no concrete current-head risks identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main changes: backporting tool truncation finish-reason handling and adding non-streaming coverage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@logprobz logprobz closed this Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant