Skip to content

[Kimi-k2.5] fix(regen): carry the target's reasoning into the regenerated history#864

Draft
WindChimeRan wants to merge 1 commit into
vllm-project:mainfrom
WindChimeRan:fix/regen-carry-reasoning-content
Draft

[Kimi-k2.5] fix(regen): carry the target's reasoning into the regenerated history#864
WindChimeRan wants to merge 1 commit into
vllm-project:mainfrom
WindChimeRan:fix/regen-carry-reasoning-content

Conversation

@WindChimeRan

@WindChimeRan WindChimeRan commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Warning!! This PR is from static code analysis, not tested end2end on kimi2.5!

Purpose

On-policy regeneration appends each reply to the running prefix as content alone, dropping the reasoning the server just parsed out. Chat templates read that field to decide whether a turn's reasoning is re-rendered into the next prompt, so every turn after the first was conditioned on a prefix serving would not produce.

Take a two-step tool call on Kimi K2.5. The target reasons, calls get_weather, and the second request has to re-render that call as history:

// what regen sends back for turn 2
{"role": "assistant", "content": "", "tool_calls": [{"id": "call_1", ...}]}
// before -- the reasoning behind the call is gone
<|im_assistant|>assistant<|im_middle|><think></think><|tool_calls_section_begin|>...

// after -- it survives, as it does when serving
<|im_assistant|>assistant<|im_middle|><think>Need the weather tool for Tokyo.</think><|tool_calls_section_begin|>...

Kimi K2.5 preserves reasoning for exactly the in-flight tool loop and reads it only from reasoning_content -- no fallback. Qwen3 hid the bug because its template splits <think> back out of content when the field is missing, so the same defect is invisible there and silent here.

This forwards the field. It reads either response spelling (current vLLM answers with reasoning, older builds used reasoning_content) and sends the older one, which current vLLM normalizes and older builds pass through untouched. Without --reasoning-parser there is no field on the response, nothing is carried, and behaviour is unchanged.

Tests

One test, on the tool-call path -- the branch where a real template actually loses the data.

Red without the source change, green with it:

E  assert {"role": "assistant", "content": "", "tool_calls": [...]}
        == {"role": "assistant", "content": "", "tool_calls": [...],
            "reasoning_content": "Need the weather tool."}
pytest tests/unit/scripts/test_response_regeneration.py -q
44 passed

ruff check, ruff format --check, and mypy --check-untyped-defs (174 files) all pass.

Not yet verified end to end against a live thinking model -- the round trip is traced through vLLM's request normalization and template rendering, but no two-turn request has been run to watch the field survive. Draft until that is done.

Checklist

I have filled in:

  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan/results, such as providing test command and pasting the results.
  • (Optional) The necessary documentation update.
  • I (a human) have written or reviewed the code in this pr to the best of my ability.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 205ed3f4-9b8f-429d-95b5-fb6fc46d978b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ 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.

@mergify

mergify Bot commented Jul 25, 2026

Copy link
Copy Markdown

Merge Protections

🔴 1 of 1 protections blocking · waiting on 👀 reviews

Protection Waiting on
🔴 Require approval from approved reviewers list 👀 reviews

🔴 Require approval from approved reviewers list

Waiting for any of

  • approved-reviews-by = dsikka
  • approved-reviews-by = fynnsu
  • approved-reviews-by = orestis-z
  • approved-reviews-by = rahul-tuli
  • approved-reviews-by = shanjiaz
This rule is failing.

All pull requests must have at least one approving review from a member of the approved reviewers list before merging.

  • any of:
    • approved-reviews-by = dsikka
    • approved-reviews-by = fynnsu
    • approved-reviews-by = orestis-z
    • approved-reviews-by = rahul-tuli
    • approved-reviews-by = shanjiaz

@WindChimeRan WindChimeRan changed the title fix(regen): carry the target's reasoning into the regenerated history [Kimi-k2.5] fix(regen): carry the target's reasoning into the regenerated history Jul 25, 2026
On-policy regeneration appended each reply to the running prefix as
`content` alone, dropping the `reasoning` the server had just parsed out.
Chat templates read that field to decide whether a turn's reasoning is
re-rendered into the next prompt, so the next turn was conditioned on a
prefix serving would not produce.

Qwen3 hid this: its template falls back to splitting `<think>` out of
`content` when the field is missing. Kimi K2.5 has no fallback and
preserves reasoning for exactly the in-flight tool loop, so a tool-calling
conversation lost the reasoning behind every call.

Forward the field instead. Reads either response spelling and sends the
older one, which current vLLM normalizes and older builds pass through. A
no-op without `--reasoning-parser`, since there is then no field to carry.

Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
@WindChimeRan
WindChimeRan force-pushed the fix/regen-carry-reasoning-content branch from 6b602fd to 20bdc51 Compare July 25, 2026 23:54
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