Conversation
This was referenced Sep 23, 2026
Contributor
|
Hi @innoprej, Thank you for your contribution and for taking the time to submit this pull request. Our team is currently reviewing your changes and we will reach out if we need any further information. |
MiloszSobczyk
requested changes
Sep 28, 2026
innoprej
force-pushed
the
fix/confirmation-scan-last-user-turn
branch
from
September 28, 2026 13:48
ffe433d to
265177c
Compare
innoprej
force-pushed
the
fix/confirmation-scan-last-user-turn
branch
from
September 29, 2026 22:23
265177c to
1153f0a
Compare
MiloszSobczyk
self-requested a review
September 30, 2026 07:45
MiloszSobczyk
approved these changes
Sep 30, 2026
Member
|
Helllo, could you please merge the newest changes that were introduced in the code? Afterwards, please ensure that the code still works correctly. |
- Stop confirmation lookup at the latest user message with content so an earlier approval cannot resume a tool after a later unrelated message. - Skip contentless internal events, including compaction and state-only updates, so they do not hide an approval that has not been handled. - Keep the recorded-response guard and add regressions for later messages, contentless events, and present-but-empty content.
innoprej
force-pushed
the
fix/confirmation-scan-last-user-turn
branch
from
September 30, 2026 16:36
1153f0a to
a87ee95
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
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
fix(core): only resume tool confirmations for calls this agent emitted, v1.8.0)2. Or, if no issue exists, describe the change:
Problem:
RequestConfirmationLlmRequestProcessor.findMostRecentConfirmationswalks the session backwards and skips every user event that carries no function responses. An approval (adk_request_confirmationfunction response) answered several turns earlier is therefore matched again on every later LLM call. ThealreadyResumedIdscheck only protects calls that left an agent-authored function response, so when the approved call did not produce one, the tool is silently re-executed with its original arguments on the next, unrelated user message — and again on every turn after that. A deterministic way to get there: the resumed execution fails asynchronously (a customBaseToolwhoserunAsyncthrows or returns a failingSingle, or aFunctionToolmethod returning a failingSingle/Maybe) and noonToolErrorCallbacksupplies a response;assembleEvent(...).onErrorReturn(...)logs the error and yields no event, so no function response is ever persisted. A synchronous exception from aFunctionToolmethod does not get there:FunctionTool.runAsynccatches it and returns an error response, which is saved.ADK Python's
_RequestConfirmationLlmRequestProcessor(flows/llm_flows/tools/_confirmation.py) stops at the most recent user-authored event and returns when that event has no function responses, so a plain text turn ends the search there. This change follows that boundary for user events with content, while preserving approvals across contentless internal events as described below.Reproduction (added as a unit test): session = original tool call → confirmation requested →
adk_request_confirmationcall → user approval → no tool response → later plain-text user turn. Before this change the processor returns a resumedecho_toolresponse event on the next LLM call; expected: nothing to resume.Solution:
Stop the scan at the most recent user event with content: return empty if it has no request-confirmation responses. Skip user-authored events with absent content, including compaction events and state-delta-only events. An empty
Contentis still a message and ends the scan. This narrow exception differs from the current Python processor and prevents an internal event from hiding an unhandled approval. The existingalreadyResumedIdsguard still prevents a call with a recorded agent response from being resumed again.A later user message with content ends the search even if the previous approval was not handled, for example after a short-circuiting
beforeRunCallback/beforeAgentCallback. Contentless internal events do not end that search. Parallel-branch scoping (eventsOnCurrentBranch) and confirmation provenance checks are unchanged.Testing Plan
Unit Tests:
mvn -pl core test -Dtest=RequestConfirmationLlmRequestProcessorTest— 21 tests, 0 failures in both the default and basic executions (Microsoft OpenJDK 17.0.19, Windows 11, Maven 4.0.0-rc-3 viamvnw). The original two regressions fail on unchangedmain. The compaction and state-delta-only regressions fail on the preceding PR head (ffe433d5), returning no resumed tool event, and pass with this change. Two further tests verify that an intervening text message and present-but-empty content still end the scan.Full core, with
-Dmaven.test.failure.ignore=trueso all six executions finish:default-testandbasiceach run 1889 tests, with 24 skipped, 0 errors and 1 failure:LocalSkillSourceTest.testListResources, the previously reproduced Windows path-separator failure onmain(tracked in #1541, with a separate fix in #1542). Its source and test are unchanged by this PR. The remaining executions run 2, 24 (1 skipped), 1 and 0 tests, with no failures or errors; the zero-test execution is tracked in #1557. The flag allows completion and does not mean the full suite passed.Manual End-to-End (E2E) Tests:
Observed originally in an application with a
requireConfirmationtool: a user approved a pending call, the resumed execution was aborted, and the next unrelated question re-ran the tool with the original arguments. With this change the follow-up question is answered without touching the tool. The review follow-up was tested at the processor level, using an actualLlmEventSummarizerevent and a state-delta-only fixture. A state-only resume throughRunnerwas not tested end to end; the public base requires a non-null user message.Checklist
Additional context
The
alreadyResumedIdsguard remains necessary for repeated LLM steps within the same invocation. Before this change, Java also relied on it after later user turns.The confirmation policy change in #1512 leaves
findMostRecentConfirmationsuntouched. That PR is currently closed without merging.