Repository navigation
fix(confirmation): run a confirmed tool once per approval - #7435
Open
Vivek1106-04 wants to merge 1 commit into
Open
Vivek1106-04 wants to merge 1 commit into
Vivek1106-04 wants to merge 1 commit into
Conversation
Step 2 of the request confirmation processor looked for the original call's result only after the last user event. Delivering the same confirmation response again (a client retry, a resubmitted form) makes it the last user event, so the result of the first resume was missed and the tool ran again with the original arguments. Look for the result after the first user event that answered each confirmation instead. The "requires confirmation" placeholder result is written before that answer, so the first approval still runs. Closes: google#7434
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
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
1. Link to an existing issue (if applicable):
Problem:
Step 2 of
_RequestConfirmationLlmRequestProcessor.run_asyncdrops confirmations that were already consumed by looking for the original call's function response only in events after the last user event. If the same confirmation response is delivered again (a client retry, a double-submitted approval button, a redelivered A2A message), it becomes the last user event, and the result of the first resume is now before it. The check misses it, and the tool runs again with the original arguments. One approval can run arequire_confirmationtool any number of times, and a repeated rejection writes another rejected result each time.Solution:
Track, per confirmation, the first user event that answered it. A confirmation counts as consumed when the original call has a non-user function response after that event. The "requires confirmation" placeholder result is written before the answer, so it is not counted and the first approval still runs. Responses after the last user event come after the first answer too, so the existing same-turn dedup still applies.
With this change, a repeated confirmation response no longer runs the tool. The resume logic in
_resume.pythen re-dispatches the original call, so the gated tool asks for a fresh confirmation instead of running. I kept the change in the processor and did not touch the resume routing.Testing Plan
Unit Tests:
TestHITLConfirmationReplay.test_repeated_confirmation_does_not_rerun_tool[True|False]runs the query, sends the confirmation, then sends the same confirmation again. It checks that the tool ran once (approved) or not at all (rejected), and that the replay only produced a new confirmation request. The runner is created withcheck_invariants=False, because the repeated confirmation response is itself a second response to the same call, which the invariant checker rejects by design.With the source change reverted:
With the change:
The 7 failures are
test_gke_code_executor.py(5) andtest_import_loading.py(2), and they fail the same way on a cleanmainin my environment.pre-commit runon the changed files passes.Manual End-to-End (E2E) Tests:
InMemoryRunner, a fakeBaseLlmthat calls the tool once and then answers with text, andFunctionTool(transfer, require_confirmation=True). The same approval is sent three times:Checklist
Additional context
adk-go had the same bug and the same fix is in google/adk-go#1753. The Go version can bound the search by the confirmation request event, because there the placeholder result is written before the request. In Python the request comes first, so the bound here is the first answer instead.