fix: yield tool responses before confirmations#765
Open
gustafvh wants to merge 1 commit into
Open
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request modifies the tool execution flow in base_flow.go to yield function responses before confirmation requests. This change ensures that tool results are persisted by consumers even when execution pauses for user approval. The test suites in function_test.go and set_test.go have been updated to reflect this new event ordering, and a new test case was added to specifically verify the sequence. I have no feedback to provide as there were no review comments.
|
@mazas-google tagging you here |
A consumer that pauses on receiving an adk_request_confirmation event (typically to await human approval) would never see the merged function-response event for tools that already executed in the same step. Yield completed responses first so they are persisted before the human-confirmation pause. Fixes google#759
radhus
force-pushed
the
fix/function-responses-before-confirmation
branch
from
80e2dcd to
b087891
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
2 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.
Link to Issue or Description of Change
Problem
runOneStepcurrently yields a generatedadk_request_confirmationevent before yielding the merged function-response event from tool execution.Consumers commonly pause as soon as they receive a tool confirmation request so they can ask a user to approve or reject the pending action. If such a consumer stops iterating at that boundary, the confirmation request is yielded and persisted, but the already completed function-response event may never be persisted to session history.
Solution
Yield the merged function-response event before yielding the generated
adk_request_confirmationevent, so completed tool results are persisted before execution pauses for human confirmation.Testing Plan
TestToolConfirmationYieldsFunctionResponseBeforeConfirmationverifying a consumer that stops at the confirmation event has already seen the tool function response.TestToolConfirmationfixture order for the new yield order.TestMCPToolSetConfirmationfixture order for the new yield order.go test ./internal/llminternal ./tool/functiontool ./tool/mcptoolsetAlignment with adk-python
This keeps the session-history invariant that completed tool responses are available before the framework pauses for human approval of a confirmation request.