fix: enforce denied tool confirmations centrally

[AnvitDevadiga]

What changed

Enforce ToolConfirmation decisions at the ADK framework boundary.

• Declined confirmations now return a framework-generated denial response.
• Denied tools are never passed to BaseTool.run_async.
• Approved tools retain the existing execution path.
• Mixed approved and denied batches are handled independently.

Why

Custom BaseTool implementations can currently execute after a user clicks Decline unless each tool reimplements the denial check. This fixes the issue described in #7148 and makes the documented confirmation flow default-deny.

Testing

• 39 targeted confirmation, tool, and runner tests pass.
• Ruff checks pass.

Closes #7148

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[babyblueviper1]

Good fix — moving denial enforcement to the framework boundary instead of trusting each BaseTool to reimplement it is the right shape (matches the general lesson: a verdict object is only as strong as the code path that's forced to consume it, not the code path that happens to check it).

One adjacent question worth asking before merge, not asserted as a bug since I haven't reproduced it against this exact tree: does the resume path guard against the same function_call_id/ToolConfirmation being submitted more than once? If a client can call the resume endpoint twice with an identical confirmed=True payload for a function_call_id that's already been executed, does _resolve_confirmation_targets re-execute it, or is there state elsewhere marking that confirmation as consumed?

Reason I ask: we hit the identical shape in our own verdict-issuing service — a pure function computing a decision reference had no state, so an identical still-valid verdict could be replayed to authorize a second dispatch (caught via a negative-control test explicitly checking for replay, not by inspection). The fix there was a small addition on top of the same "enforce at the boundary" pattern this PR already uses: a primary-keyed consumed-set with atomic check-and-mark, so a race between two identical resume calls has exactly one winner instead of two executions. If tools_to_resume_with_confirmation here is keyed by function_call_id and nothing marks an id as spent after this code path runs, the same gap likely exists — cheap to close now while the boundary-enforcement code is already being touched, much easier than adding it after the fact once callers start depending on today's semantics.

[Tardfyou]

Following @surajksharma07's request in #7148, I validated the proposed shared-caller workaround locally on google-adk==2.9.1 using the original deterministic fake-LLM/custom-BaseTool harness. The only effect is a temporary marker file; no external model or service is contacted.

Results:

Caller	User verdict	Tool executions after verdict	Marker written
Unmodified 2.9.1	confirmed=False	1	yes
Caller denies confirmed=False before run_async	confirmed=False	0	no
Same workaround	confirmed=True	1	yes
Unmodified 2.9.1	confirmed=True	1	yes

All four assertions passed. This validates the suggested caller-level workaround for the reported custom-tool path.

One distinction matters before treating this as validation of this PR: the current head 3557a41 filters denied calls in tools/_confirmation.py, whereas the workaround I tested guards _call_tool_async (in _tool_caller.py in release 2.9.1). I have not run this PR head, so this comment is not an end-to-end verification of #7151. Please retain a runner-level regression with a custom BaseTool that does not inspect the verdict, plus the approved control. The replay question above is also outside the scope of my four-case check.

[AnvitDevadiga]

Thanks for pointing this out. You’re right that the existing event-history deduplication does not provide an atomic check-and-mark guarantee for concurrent resume requests.

I updated the implementation so each confirmation is claimed atomically by the invocation context before dispatch. A repeated or concurrent submission for the same function_call_id is now ignored, ensuring the tool can execute at most once per confirmation.

I also added the protection for denied confirmations, since those should be consumed exactly once as well. Syntax and diff validation pass; the targeted test suite is currently blocked in this environment because PyPI access is unavailable.

[babyblueviper1]

Verified this against the actual PR head (6627ed4), not just the comment thread — the fix is the right shape: _consumed_tool_confirmation_ids is a primary-keyed set on InvocationContext, claimed under _tool_confirmation_consume_lock before dispatch, exactly the atomic check-and-mark pattern that closes the race from my original question (a confirmed=True payload for the same function_call_id submitted twice, one winner).

I went one step further before calling it closed: does this survive a crash/resume into a new process? _consumed_tool_confirmation_ids is a Pydantic PrivateAttr on InvocationContext, and private attrs don't round-trip through model serialization — so a fresh process resuming a persisted session gets an empty consumed-set. Read through _confirmation.py's full pipeline to check whether that reopens anything, and it doesn't: Step 2 (event-history dedup, responded_fc_ids from the persisted event log) runs before Step 3's in-memory claim and drops any confirmation that already has a recorded function_response — that check is durable across process boundaries because the event log is what gets persisted, not the in-memory set. So the in-memory set only has to be correct for races within one process's lifetime on a still-unanswered confirmation, which is exactly what it does. No residual gap at the resume boundary — the two checks are at different scopes and compose correctly.

What the PR doesn't have yet: a direct regression test for the concurrency race itself. The diff's two test changes just update expected deny-path messages — nothing exercises two simultaneous calls into _consume_tool_confirmation (or two concurrent resume calls) for the same function_call_id to assert exactly one wins. Happy to write that against 6627ed4 (asyncio.gather of two confirm-and-dispatch attempts, assert one executes and one no-ops) — same pattern we run internally for our own consumed-set on decision-reference reuse. Say the word and I'll open it as a PR.