Skip to content

fix(confirmation): run a confirmed tool once per approval - #7435

Open
Vivek1106-04 wants to merge 1 commit into
google:mainfrom
Vivek1106-04:fix/confirmation-replay
Open

Vivek1106-04 wants to merge 1 commit into
google:mainfrom
Vivek1106-04:fix/confirmation-replay

Conversation

@Vivek1106-04

Copy link
Copy Markdown

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

Problem:

Step 2 of _RequestConfirmationLlmRequestProcessor.run_async drops 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 a require_confirmation tool 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.py then 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:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

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 with check_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:

FAILED ...::TestHITLConfirmationReplay::test_repeated_confirmation_does_not_rerun_tool[True]
E     AssertionError: assert 2 == 1
FAILED ...::TestHITLConfirmationReplay::test_repeated_confirmation_does_not_rerun_tool[False]
E     At index 0 diff: {'error': 'This tool call is rejected.'} != {'error': 'This tool call requires confirmation, please approve or reject.'}

With the change:

$ pytest tests/unittests/runners/test_run_tool_confirmation.py
12 passed

$ pytest tests/unittests -n auto
7 failed, 17888 passed, 88 skipped, 25 xfailed, 2 xpassed

The 7 failures are test_gke_code_executor.py (5) and test_import_loading.py (2), and they fail the same way on a clean main in my environment. pre-commit run on the changed files passes.

Manual End-to-End (E2E) Tests:

InMemoryRunner, a fake BaseLlm that calls the tool once and then answers with text, and FunctionTool(transfer, require_confirmation=True). The same approval is sent three times:

main:
-- user approves
  >>> TRANSFER EXECUTED amount=100 (total executions=1)
-- same approval delivered again (client retry)
  >>> TRANSFER EXECUTED amount=100 (total executions=2)
-- and again
  >>> TRANSFER EXECUTED amount=100 (total executions=3)

this PR:
-- user approves
  >>> TRANSFER EXECUTED amount=100 (total executions=1)
-- same approval delivered again (client retry)
  confirmation requested, id adk-7dc7460e-02ec-4d96-8d4c-c1bce9613289
-- and again
  confirmation requested, id adk-346b1c64-4da9-4c24-8d67-1b626ca8c652
executions: 1

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

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.

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
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.

Tool confirmation: a repeated approval runs the confirmed tool again

2 participants