fix: replace blunt pop with assistant message rewriting in _separate_tool_calls#5303
Open
jaideepr97 wants to merge 3 commits intollamastack:mainfrom
Open
fix: replace blunt pop with assistant message rewriting in _separate_tool_calls#5303jaideepr97 wants to merge 3 commits intollamastack:mainfrom
jaideepr97 wants to merge 3 commits intollamastack:mainfrom
Conversation
jaideepr97
requested review from
ashwinb,
bbrowning,
cdoern,
ehhuang,
franciscojavierarceo,
leseb,
mattf and
raghotham
as code owners
meta-cla
bot
added
the
CLA Signed
Contributor
Author
|
@grs Please take a look at this when you get a second, would appreciate your inputs! |
grs
approved these changes
Mar 26, 2026
gyliu513
reviewed
Mar 27, 2026
| @@ -0,0 +1,348 @@ | |||
| # Copyright (c) Meta Platforms, Inc. and affiliates. | |||
Contributor
There was a problem hiding this comment.
Instead of creating a new file, can you put your test case to tests/unit/providers/inline/responses/builtin/responses/test_streaming.py ?
Contributor
Author
There was a problem hiding this comment.
updated, thanks!
Contributor
…ltiple MCP approval tool calls The _separate_tool_calls() method called next_turn_messages.pop() inside the tool call loop, once per tool call needing approval. With N tool calls requiring approval, pop() fired N times — the first correctly removed the assistant message, but subsequent pops destroyed unrelated conversation messages (user message, system message, etc.). Track whether a pop is needed with a boolean flag and execute it at most once after the tool call loop completes. Fixes: llamastack#5301 Signed-off-by: Jaideep Rao <jrao@redhat.com> Made-with: Cursor
…tool_calls The _separate_tool_calls() method called next_turn_messages.pop() inside the tool call loop, once per tool call needing approval. With N such tool calls, pop() fired N times — the first correctly removed the assistant message, but subsequent pops destroyed unrelated conversation messages. Replace the per-tool-call pop with intelligent assistant message handling: - All deferred/denied: pop the assistant message entirely (no tool calls to preserve context for) - Mixed (some executed, some deferred): replace the assistant message with one containing only the executed tool calls, keeping the conversation history coherent with the tool results that follow - All executed: leave the assistant message untouched Fixes: llamastack#5301 Signed-off-by: Jaideep Rao <jrao@redhat.com> Made-with: Cursor
Consolidate the regression tests for the pop() bug fix from the standalone test_approval_pop_bug.py into the existing test_streaming.py file per reviewer feedback. Signed-off-by: Jaideep Rao <jrao@redhat.com> Made-with: Cursor Signed-off-by: Jaideep Rao <jrao@redhat.com>
jaideepr97
force-pushed
the
fix/approval-pop-history-corruption
branch
from
68c8651 to
7ac4102
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch was generated by Claude.
Summary
Fixes #5301
_separate_tool_calls()callednext_turn_messages.pop()inside the tool call loop, once per tool call needing approval. With N such tool calls,pop()fired N times — the first correctly removed the assistant message, but subsequent pops destroyed unrelated conversation messages (user message, system message, etc.).Additionally, even in the single-tool-call case, the pop removed the entire assistant message including tool calls that were approved and executed — leaving orphaned tool results in the conversation history and model context.
Fix
Replace the per-tool-call pop with intelligent assistant message handling based on what was actually executed:
Test plan