Skip to content

fix(core): stop re-applying a tool confirmation after a later user turn - #1534

Open
innoprej wants to merge 1 commit into
google:mainfrom
innoprej:fix/confirmation-scan-last-user-turn
Open

innoprej wants to merge 1 commit into
google:mainfrom
innoprej:fix/confirmation-scan-last-user-turn

Conversation

@innoprej

@innoprej innoprej commented Sep 22, 2026 •

Copy link
Copy Markdown

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):

2. Or, if no issue exists, describe the change:

Problem:

RequestConfirmationLlmRequestProcessor.findMostRecentConfirmations walks the session backwards and skips every user event that carries no function responses. An approval (adk_request_confirmation function response) answered several turns earlier is therefore matched again on every later LLM call. The alreadyResumedIds check 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 custom BaseTool whose runAsync throws or returns a failing Single, or a FunctionTool method returning a failing Single/Maybe) and no onToolErrorCallback supplies a response; assembleEvent(...).onErrorReturn(...) logs the error and yields no event, so no function response is ever persisted. A synchronous exception from a FunctionTool method does not get there: FunctionTool.runAsync catches 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_confirmation call → user approval → no tool response → later plain-text user turn. Before this change the processor returns a resumed echo_tool response 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 Content is 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 existing alreadyResumedIds guard 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:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally. The target class passes; the full suite retains one unrelated Windows failure in each main execution, detailed below.

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 via mvnw). The original two regressions fail on unchanged main. 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=true so all six executions finish: default-test and basic each run 1889 tests, with 24 skipped, 0 errors and 1 failure: LocalSkillSourceTest.testListResources, the previously reproduced Windows path-separator failure on main (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 requireConfirmation tool: 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 actual LlmEventSummarizer event and a state-delta-only fixture. A state-only resume through Runner was not tested end to end; the public base requires a non-null user message.

Checklist

  • I have read the CONTRIBUTING.md document.
  • My pull request contains a single commit.
  • 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. See the full-suite result above for the existing Windows failure.
  • I have manually tested my changes end-to-end. The review follow-up was verified at the processor level.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

The alreadyResumedIds guard 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 findMostRecentConfirmations untouched. That PR is currently closed without merging.

@hemasekhar-p

Copy link
Copy Markdown
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.

@innoprej
innoprej force-pushed the fix/confirmation-scan-last-user-turn branch from ffe433d to 265177c Compare September 28, 2026 13:48
@innoprej
innoprej force-pushed the fix/confirmation-scan-last-user-turn branch from 265177c to 1153f0a Compare September 29, 2026 22:23
@MiloszSobczyk
MiloszSobczyk self-requested a review September 30, 2026 07:45
@MiloszSobczyk

Copy link
Copy Markdown
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
innoprej force-pushed the fix/confirmation-scan-last-user-turn branch from 1153f0a to a87ee95 Compare September 30, 2026 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Approved tool call re-runs on every later user turn if it never got a function response

3 participants