fix: preserve prior-turn analysis messages in Harmony multi-turn conversations#35826
Conversation
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in auto_drop_analysis_messages where analysis messages from previous turns in a multi-turn conversation were incorrectly dropped. The new logic correctly identifies the current turn and only drops analysis messages within that turn, preserving necessary context from prior turns. The added tests cover the fix and various multi-turn scenarios well. I've identified one potential critical issue where a user message could be inadvertently dropped and have provided a suggestion to make the logic more robust.
| return [ | ||
| msg | ||
| for i, msg in enumerate(msgs) | ||
| if not ( | ||
| current_turn_start <= i < last_final_in_turn and msg.channel == "analysis" | ||
| ) | ||
| ] |
There was a problem hiding this comment.
The current logic for dropping analysis messages doesn't check the message role. This could lead to dropping a user message if it happens to have channel="analysis", which would be a critical bug as it modifies user input. While analysis messages are typically not from the user, the function should be robust against this possibility.
To prevent this, I suggest adding a check to ensure we don't drop messages with the user role. The existing test test_drops_non_assistant_analysis_messages uses a TOOL role, which would still pass with this change.
| return [ | |
| msg | |
| for i, msg in enumerate(msgs) | |
| if not ( | |
| current_turn_start <= i < last_final_in_turn and msg.channel == "analysis" | |
| ) | |
| ] | |
| return [ | |
| msg | |
| for i, msg in enumerate(msgs) | |
| if not ( | |
| current_turn_start <= i < last_final_in_turn | |
| and msg.channel == "analysis" and msg.author.role != "user" | |
| ) | |
| ] |
…ersations `auto_drop_analysis_messages` was dropping ALL analysis messages before the last assistant final message, including from prior conversation turns. This prevented clients from preserving reasoning context across turns even when explicitly providing `reasoning_content`. Change the algorithm to only drop analysis messages within the current turn (after the last user message), preserving prior-turn analysis so the model can access its own previous reasoning. Fixes vllm-project#35779 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: liweiguang <codingpunk@gmail.com>
Address review feedback: add msg.author.role != "user" guard to prevent accidentally filtering non-assistant messages from the analysis channel. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: liweiguang <codingpunk@gmail.com>
674fbfd to
0142105
Compare
|
Thanks for the review! I've adopted the defensive Both commits now also include |
|
Please see comments from #35779, the current behavior is intentional as per openai spec, this is not a bug. |
|
Closing — the current behavior is intentional per OpenAI spec (see #35779 discussion). Thanks for the clarification! |
Summary
auto_drop_analysis_messagesto only drop analysis messages within the current turn (after the last user message), preserving prior-turn reasoning contextreasoning_contentfrom prior conversation turnsRoot Cause
When a client sends a multi-turn conversation with
reasoning_contenton prior assistant messages:[ {"role": "user", "content": "Pick a secret number..."}, {"role": "assistant", "content": "OK, I picked.", "reasoning_content": "I will pick 742."}, {"role": "user", "content": "What is the secret number?"} ]The analysis message from the first turn (containing "742") was dropped because the old code dropped ALL analysis messages before the last assistant final — regardless of turn boundaries. The model could not access its own prior reasoning.
Fix
Changed the algorithm to:
Test Plan
test_preserves_prior_turn_analysis_messages— exact scenario from the issuetest_drops_current_turn_analysis_but_preserves_prior— multi-turn with both prior and current analysistest_preserves_analysis_when_no_final_in_current_turn— ongoing reasoning not droppedTestAutoDropAnalysisMessagestests pass unchanged (verified via logic simulation)Fixes #35779