Skip to content

fix(flows): re-execute orphaned sibling calls on confirmation resume - #6733

Open
chelsealong wants to merge 2 commits into
google:mainfrom
chelsealong:fix-6732-parallel-confirmation-sibling-loss
Open

fix(flows): re-execute orphaned sibling calls on confirmation resume#6733
chelsealong wants to merge 2 commits into
google:mainfrom
chelsealong:fix-6732-parallel-confirmation-sibling-loss

Conversation

@chelsealong

Copy link
Copy Markdown
Contributor

Closes #6732

Problem

When a model turn contains a confirmation-gated FunctionTool call in parallel with ungated calls, the ungated siblings' results can be lost across the confirmation pause.

The merged function_response_event for that turn (carrying the sibling's real result plus the gated tool's "requires confirmation" placeholder) is built and yielded after the adk_request_confirmation event by _postprocess_handle_function_calls_async. A caller that stops consuming the event stream at the confirmation event (the natural thing to do while waiting for user input, e.g. an SSE/AG-UI bridge) never asks for the next event, so that response event — and the runner's append_event for it — never happens.

On resume, _RequestConfirmationLlmRequestProcessor._resolve_confirmation_targets only re-executes the confirmed tool. Session history is left with the sibling's function_call and no matching function_response, and the next LLM call (Gemini) returns an empty reply — even though the confirmed (possibly destructive) tool already executed.

Fix

_RequestConfirmationLlmRequestProcessor.run_async now also scans for sibling function calls that share a model turn with a confirmed call and still have no function_response anywhere in history (_get_orphaned_sibling_function_calls in src/google/adk/flows/llm_flows/request_confirmation.py), and re-executes them alongside the confirmed tool. This keeps the turn's function calls and responses in sync so the resumed continuation never sees a dangling function_call.

This is the minimal of the two fixes discussed in the issue (re-execute unresolved siblings on resume, vs. reordering the pause-time yield so the response event persists before the confirmation event) — it's a single, local change to the resume path and does not touch the pause-time event ordering or its existing tests.

Follow-up fix: the first version of _get_orphaned_sibling_function_calls decided a call was an "ungated orphan" based only on "no function_response anywhere in history" + "registered tool" — it never checked whether the sibling itself still required (and had not yet received) its own confirmation. When a turn has two confirmation-gated calls and only one is confirmed, this swept the still-pending sibling into re-execution, which re-triggered request_confirmation bookkeeping for a confirmation that was never resolved (minting a fresh adk_request_confirmation call while the original request went orphaned). The helper now also checks tool.check_require_confirmation (plus dynamic-confirmation history via the extracted _compute_dynamically_requested_fc_ids) for each candidate sibling and skips ones that still require confirmation, leaving their own resume to a later turn once they're actually confirmed.

Testing

Added test_request_confirmation_processor_reexecutes_orphaned_sibling in tests/unittests/flows/llm_flows/test_request_confirmation.py, which builds session history for a turn with one gated call (unconfirmed) and one real ungated sibling call and no function_response event for that turn (reproducing the lost-response scenario), then confirms the gated tool and asserts the resumed processor's merged response contains a function_response for both tools.

Verified the test fails without the fix (git stash the source change, run the test):

AssertionError: assert {'mock_tool'} == {'mock_tool', 'sibling_tool'}

Added test_request_confirmation_processor_leaves_gated_sibling_pending, covering the follow-up fix: two gated tools in the same turn, only one confirmed. Verified this test fails against the pre-follow-up-fix source (reproduced via a standalone repro script run against git show HEAD:src/.../request_confirmation.py, since the test module itself imports the newly extracted _compute_dynamically_requested_fc_ids helper):

AssertionError: BUG REPRODUCED: gated sibling was swept in: {'gated_sibling_tool', 'mock_tool'}

and that it also minted a fresh requested_tool_confirmations entry for the still-pending sibling. With the follow-up fix applied, only mock_tool's response is present and no confirmation bookkeeping is re-triggered.

With the fix:

$ pytest tests/unittests/flows/llm_flows/test_request_confirmation.py tests/unittests/runners/test_run_tool_confirmation.py -q
28 passed, 12 warnings in 2.93s

Full unit suite:

$ pytest tests/unittests -n auto -q
12023 passed, 88 skipped, 25 xfailed, 1 xpassed, 3162 warnings, 24 subtests passed in 148.95s

pre-commit run --files src/google/adk/flows/llm_flows/request_confirmation.py tests/unittests/flows/llm_flows/test_request_confirmation.py passes (ruff, isort, pyink, addlicense, ADK compliance checks, codespell).

AI assistance disclosure

This change was written with AI assistance (Claude Code / Anthropic).

When a model turn pairs a confirmation-gated tool call with ungated
sibling calls, the merged function_response_event for that turn can fail
to persist across the pause (e.g. a caller stops consuming the event
stream right at the adk_request_confirmation event, so the response
event that _postprocess_handle_function_calls_async yields afterwards is
never reached). On resume, _resolve_confirmation_targets only
re-executed the confirmed tool, leaving the sibling's function_call in
history with no matching function_response, which makes the next LLM
call return an empty reply.

_RequestConfirmationLlmRequestProcessor now also finds sibling calls
from the same turn that still have no function_response and re-executes
them alongside the confirmed tool.

Fixes google#6732
_get_orphaned_sibling_function_calls treated any sibling call lacking a
function_response as an ungated orphan, without checking whether it was
itself still awaiting its own confirmation. When two confirmation-gated
calls land in the same turn and only one is confirmed, this re-triggered
request_confirmation bookkeeping for the still-unanswered sibling,
minting a fresh adk_request_confirmation call while the original
request was orphaned. The helper now checks
tool.check_require_confirmation (and dynamic confirmation history) for
each candidate and skips siblings that still require confirmation.

@varunbiluri varunbiluri left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sibling is not actually unexecuted in the reported failure: the issue states it executes before the pause, but its merged response event is not persisted when the consumer stops iterating. Re-executing it on resume therefore gives at-least-twice behavior and can duplicate arbitrary side effects (send a message, charge a card, mutate a record). The regression test uses a pure string-returning sibling, so it does not expose this. Please fix persistence/event ordering (or persist/synthesize the already-produced result) rather than invoking an already-executed tool again; the resume path cannot safely infer that absence of a persisted means absence of execution.

@varunbiluri varunbiluri left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sibling is not actually unexecuted in the reported failure: the issue states that it executes before the pause, but its merged response event is not persisted when the consumer stops iterating. Re-executing on resume therefore gives at-least-twice behavior and can duplicate arbitrary side effects such as sending a message, charging a card, or mutating a record. The regression test uses a pure string-returning sibling and does not expose this. Please fix persistence or event ordering, or persist the already-produced result, rather than invoking an already-executed tool again; absence of a persisted function response does not imply absence of execution.

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.

Parallel ungated tool results are lost across a tool-confirmation pause, yielding an empty model reply after approval

3 participants