Skip to content

Commit 7ec671e

Browse files
committed
fix(tools): fail closed on non-boolean confirmation predicates
1 parent f78149a commit 7ec671e

6 files changed

Lines changed: 94 additions & 17 deletions

File tree

‎src/google/adk/tools/function_tool.py‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import re
2121
from typing import Any
2222
from typing import Callable
23-
from typing import cast
2423
from typing import Iterable
2524
from typing import Optional
2625
from typing import TYPE_CHECKING
@@ -317,10 +316,14 @@ async def check_require_confirmation(
317316
args_to_call = self._prepare_invocation_args(
318317
preprocessed_args, tool_context
319318
)
320-
return cast(
321-
bool,
322-
await self._invoke_callable(self._require_confirmation, args_to_call),
319+
result = await self._invoke_callable(
320+
self._require_confirmation, args_to_call
323321
)
322+
# typing.cast is erased at runtime. A callable that returns None (e.g. an
323+
# unhandled branch) must not skip the confirmation gate.
324+
if isinstance(result, bool):
325+
return result
326+
return True
324327
return bool(self._require_confirmation)
325328

326329
def _is_invocation_type_error(

‎src/google/adk/tools/mcp_tool/mcp_tool.py‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -546,10 +546,14 @@ async def check_require_confirmation(
546546
args_to_call = self._prepare_callable_args(
547547
self._require_confirmation, args, tool_context
548548
)
549-
return cast(
550-
bool,
551-
await self._invoke_callable(self._require_confirmation, args_to_call),
549+
result = await self._invoke_callable(
550+
self._require_confirmation, args_to_call
552551
)
552+
# typing.cast is erased at runtime. A callable that returns None (e.g. an
553+
# unhandled branch) must not skip the confirmation gate.
554+
if isinstance(result, bool):
555+
return result
556+
return True
553557
return bool(self._require_confirmation)
554558

555559
@override

‎src/google/adk/workflow/_function_node.py‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -727,13 +727,13 @@ async def _check_require_confirmation(
727727
args_to_call = {
728728
k: v for k, v in args_to_call.items() if k in valid_params
729729
}
730-
return bool(
731-
await _invoke_callable(
732-
self.require_confirmation,
733-
args_to_call,
734-
unwrapped=pred_spec.unwrapped_func,
735-
)
730+
result = await _invoke_callable(
731+
self.require_confirmation,
732+
args_to_call,
733+
unwrapped=pred_spec.unwrapped_func,
736734
)
735+
# An omitted return must not silently bypass the confirmation gate.
736+
return True if result is None else bool(result)
737737

738738
@override
739739
async def _run_impl(

‎tests/unittests/tools/mcp_tool/test_mcp_tool.py‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1708,6 +1708,33 @@ async def _require_confirmation_func(
17081708
}
17091709
tool_context.request_confirmation.assert_called_once()
17101710

1711+
@pytest.mark.asyncio
1712+
async def test_run_async_require_confirmation_callable_none_fail_closed(
1713+
self,
1714+
):
1715+
"""An omitted predicate return must request confirmation."""
1716+
tool = MCPTool(
1717+
mcp_tool=self.mock_mcp_tool,
1718+
mcp_session_manager=self.mock_session_manager,
1719+
require_confirmation=lambda **kwargs: None,
1720+
)
1721+
tool_context = Mock(spec=ToolContext)
1722+
tool_context.tool_confirmation = None
1723+
tool_context.request_confirmation = Mock()
1724+
tool_context.actions = EventActions()
1725+
1726+
result = await tool.run_async(
1727+
args={"param1": "test_value"}, tool_context=tool_context
1728+
)
1729+
1730+
assert result == {
1731+
"error": (
1732+
"This tool call requires confirmation, please approve or reject."
1733+
)
1734+
}
1735+
tool_context.request_confirmation.assert_called_once()
1736+
assert tool_context.actions.skip_summarization is True
1737+
17111738
@pytest.mark.asyncio
17121739
async def test_run_async_require_confirmation_callable_true_no_confirmation(
17131740
self,

‎tests/unittests/tools/test_function_tool.py‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -488,6 +488,48 @@ def sample_func(arg1: str):
488488
assert result == {"received_arg": "hello"}
489489

490490

491+
@pytest.mark.asyncio
492+
async def test_check_require_confirmation_callable_none_fail_closed():
493+
"""Callable returning None must not skip the confirmation gate (#7010)."""
494+
495+
executed = []
496+
497+
def sample_func(arg1: str):
498+
executed.append(arg1)
499+
return {"received_arg": arg1}
500+
501+
def forgot_a_branch(arg1: str):
502+
if arg1 == "force":
503+
return True
504+
# Falls through and returns None.
505+
506+
tool = FunctionTool(sample_func, require_confirmation=forgot_a_branch)
507+
mock_invocation_context = MagicMock(spec=InvocationContext)
508+
mock_invocation_context._state_schema = None
509+
mock_invocation_context.session = MagicMock(spec=Session)
510+
mock_invocation_context.session.state = MagicMock()
511+
mock_invocation_context.agent = MagicMock()
512+
mock_invocation_context.agent.name = "test_agent"
513+
tool_context_mock = ToolContext(invocation_context=mock_invocation_context)
514+
tool_context_mock.function_call_id = "test_function_call_id"
515+
516+
assert (
517+
await tool.check_require_confirmation(
518+
{"arg1": "hello"}, tool_context_mock
519+
)
520+
is True
521+
)
522+
523+
result = await tool.run_async(
524+
args={"arg1": "hello"},
525+
tool_context=tool_context_mock,
526+
)
527+
assert result == {
528+
"error": "This tool call requires confirmation, please approve or reject."
529+
}
530+
assert executed == []
531+
532+
491533
@pytest.mark.asyncio
492534
async def test_run_async_parameter_filtering(mock_tool_context):
493535
"""Test that parameter filtering works correctly for functions with explicit parameters."""

‎tests/unittests/workflow/test_workflow_node.py‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,9 @@
2727
from google.adk.tools.base_tool import BaseTool
2828
from google.adk.workflow import FunctionNode
2929
from google.adk.workflow import START
30-
from google.adk.workflow._base_node import BaseNode
3130
from google.adk.workflow._node import node
3231
from google.adk.workflow._node import Node
3332
from google.adk.workflow._parallel_worker import _ParallelWorker as ParallelWorker
34-
from google.adk.workflow._retry_config import RetryConfig
3533
from google.adk.workflow._tool_node import _ToolNode as ToolNode
3634
from google.adk.workflow._workflow import Workflow
3735
from google.genai import types
@@ -577,7 +575,8 @@ def after_node(node_input):
577575

578576

579577
@pytest.mark.parametrize(
580-
"target_path,should_pause", [("/dev/tmp", False), ("/prod/db", True)]
578+
"target_path,should_pause",
579+
[("/dev/tmp", False), ("/prod/db", True), ("/home/x", True)],
581580
)
582581
@pytest.mark.asyncio
583582
async def test_node_require_confirmation_callable_predicate(
@@ -596,8 +595,10 @@ async def test_node_require_confirmation_callable_predicate(
596595
def setup_state(ctx: Context):
597596
ctx.state["path"] = target_path
598597

599-
def check_path(path: str) -> str:
598+
def check_path(path: str) -> str | None:
600599
predicate_calls.append(path)
600+
if path == "/home/x":
601+
return None
601602
return path if path.startswith("/prod") else ""
602603

603604
@node(require_confirmation=check_path)

0 commit comments

Comments
 (0)