From 9bb3b6f04928819e34ec0119276107080d4c52c1 Mon Sep 17 00:00:00 2001 From: boopathi-376 Date: Fri, 4 Sep 2026 15:58:09 +0530 Subject: [PATCH 1/3] fix(tools): check_require_confirmation fails closed on non-bool callable return cast(bool, ...) is a static-only type hint and has no effect at runtime, so a require_confirmation callable that returns None was silently treated as falsy, letting the tool run without confirmation. Replace the cast with an isinstance(result, bool) runtime check that fails closed on any non-bool return. Fixes #7010 --- src/google/adk/tools/function_tool.py | 8 +- src/google/adk/tools/mcp_tool/mcp_tool.py | 8 +- .../unittests/tools/mcp_tool/test_mcp_tool.py | 78 ++++++++++++++++++- tests/unittests/tools/test_function_tool.py | 69 +++++++++++++++- 4 files changed, 155 insertions(+), 8 deletions(-) diff --git a/src/google/adk/tools/function_tool.py b/src/google/adk/tools/function_tool.py index 266a97c2d62..0dfb0dcb430 100644 --- a/src/google/adk/tools/function_tool.py +++ b/src/google/adk/tools/function_tool.py @@ -199,10 +199,12 @@ async def check_require_confirmation( ) -> bool: if callable(self._require_confirmation): args_to_call = self._prepare_invocation_args(args, tool_context) - return cast( - bool, - await self._invoke_callable(self._require_confirmation, args_to_call), + result = await self._invoke_callable( + self._require_confirmation, args_to_call ) + if isinstance(result, bool): + return result + return True return bool(self._require_confirmation) def _is_invocation_type_error( diff --git a/src/google/adk/tools/mcp_tool/mcp_tool.py b/src/google/adk/tools/mcp_tool/mcp_tool.py index 83c426875de..7618d09fb2a 100644 --- a/src/google/adk/tools/mcp_tool/mcp_tool.py +++ b/src/google/adk/tools/mcp_tool/mcp_tool.py @@ -467,10 +467,12 @@ async def check_require_confirmation( args_to_call = self._prepare_callable_args( self._require_confirmation, args, tool_context ) - return cast( - bool, - await self._invoke_callable(self._require_confirmation, args_to_call), + result = await self._invoke_callable( + self._require_confirmation, args_to_call ) + if isinstance(result, bool): + return result + return True return bool(self._require_confirmation) @override diff --git a/tests/unittests/tools/mcp_tool/test_mcp_tool.py b/tests/unittests/tools/mcp_tool/test_mcp_tool.py index 245b22d2d61..dde78cb6f0a 100644 --- a/tests/unittests/tools/mcp_tool/test_mcp_tool.py +++ b/tests/unittests/tools/mcp_tool/test_mcp_tool.py @@ -1221,6 +1221,82 @@ async def test_run_async_require_confirmation_callable_true_no_confirmation( tool_context.request_confirmation.assert_called_once() assert tool_context.actions.skip_summarization is True + @pytest.mark.asyncio + async def test_check_require_confirmation_callable_returns_none_fails_closed( + self, + ): + """A predicate that falls off a branch (implicit None) must fail closed + and require confirmation, not silently skip it. + + Regression test for #7010: `cast(bool, ...)` is a no-op at runtime, so a + predicate returning None used to be treated as falsy and the tool would + run unconfirmed. + """ + + def forgot_a_branch(param1: str): + if param1 == "never": + return True + # Falls through here and implicitly returns None. + + tool = MCPTool( + mcp_tool=self.mock_mcp_tool, + mcp_session_manager=self.mock_session_manager, + require_confirmation=forgot_a_branch, + ) + tool_context = Mock(spec=ToolContext) + + result = await tool.check_require_confirmation( + {"param1": "test_value"}, tool_context + ) + + assert result is True + + @pytest.mark.asyncio + async def test_check_require_confirmation_callable_returns_truthy_non_bool( + self, + ): + """A truthy non-bool 'reason' return should still mean 'confirm' (no + regression for this already-working pattern).""" + + def returns_reason(param1: str): + return "amount over limit" + + tool = MCPTool( + mcp_tool=self.mock_mcp_tool, + mcp_session_manager=self.mock_session_manager, + require_confirmation=returns_reason, + ) + tool_context = Mock(spec=ToolContext) + + result = await tool.check_require_confirmation( + {"param1": "test_value"}, tool_context + ) + + assert result is True + + @pytest.mark.asyncio + async def test_check_require_confirmation_callable_returns_bool_false( + self, + ): + """A predicate explicitly returning False must still allow the tool to + run without confirmation (no regression for the ordinary bool path).""" + + def returns_false(param1: str): + return False + + tool = MCPTool( + mcp_tool=self.mock_mcp_tool, + mcp_session_manager=self.mock_session_manager, + require_confirmation=returns_false, + ) + tool_context = Mock(spec=ToolContext) + + result = await tool.check_require_confirmation( + {"param1": "test_value"}, tool_context + ) + + assert result is False + def test_init_validation(self): """Test that initialization validates required parameters.""" # This test ensures that the MCPTool properly handles its dependencies @@ -2365,4 +2441,4 @@ def test_factory_protocol_stays_runtime_checkable(self): def factory(tool_name, *, callback_context=None, **kwargs): return None - assert isinstance(factory, ProgressCallbackFactory) + assert isinstance(factory, ProgressCallbackFactory) \ No newline at end of file diff --git a/tests/unittests/tools/test_function_tool.py b/tests/unittests/tools/test_function_tool.py index eaa97531cd5..1dc73520600 100644 --- a/tests/unittests/tools/test_function_tool.py +++ b/tests/unittests/tools/test_function_tool.py @@ -350,6 +350,73 @@ def sample_func(expected_arg: str): assert result == {"received_arg": "hello"} +@pytest.mark.asyncio +async def test_check_require_confirmation_callable_returns_none_fails_closed( + mock_tool_context, +): + """A predicate that falls off a branch (implicit None) must fail closed + and require confirmation, not silently skip it. + + Regression test for #7010: `cast(bool, ...)` is a no-op at runtime, so a + predicate returning None used to be treated as falsy and the tool would + run unconfirmed. + """ + + def forgot_a_branch(arg1: str): + if arg1 == "never": + return True + # Falls through here and implicitly returns None. + + tool = FunctionTool( + lambda arg1: {"received_arg": arg1}, + require_confirmation=forgot_a_branch, + ) + result = await tool.check_require_confirmation( + {"arg1": "hello"}, mock_tool_context + ) + assert result is True + + +@pytest.mark.asyncio +async def test_check_require_confirmation_callable_returns_truthy_non_bool( + mock_tool_context, +): + """A truthy non-bool 'reason' return should still mean 'confirm' (no + regression for this already-working pattern).""" + + def returns_reason(arg1: str): + return "amount over limit" + + tool = FunctionTool( + lambda arg1: {"received_arg": arg1}, + require_confirmation=returns_reason, + ) + result = await tool.check_require_confirmation( + {"arg1": "hello"}, mock_tool_context + ) + assert result is True + + +@pytest.mark.asyncio +async def test_check_require_confirmation_callable_returns_bool_false( + mock_tool_context, +): + """A predicate explicitly returning False must still allow the tool to + run without confirmation (no regression for the ordinary bool path).""" + + def returns_false(arg1: str): + return False + + tool = FunctionTool( + lambda arg1: {"received_arg": arg1}, + require_confirmation=returns_false, + ) + result = await tool.check_require_confirmation( + {"arg1": "hello"}, mock_tool_context + ) + assert result is False + + @pytest.mark.asyncio async def test_run_async_with_tool_context_and_unexpected_argument(): """Test that run_async handles tool_context and filters out unexpected arguments.""" @@ -755,4 +822,4 @@ async def tool_with_int(flag: int): args={"flag": True}, tool_context=mock_tool_context, ) - assert result == {"type": "bool"} + assert result == {"type": "bool"} \ No newline at end of file From 88dfb727a5a0b1c653add4a58c8fb3c85221debf Mon Sep 17 00:00:00 2001 From: boopathi-376 Date: Mon, 7 Sep 2026 10:00:54 +0530 Subject: [PATCH 2/3] fix(tools): address review feedback on #7012 - Detect un-awaited awaitables returned by require_confirmation predicates and treat them as requiring confirmation, with a logger.warning explaining why. - Log a warning whenever a non-bool return is coerced to True, so a silent behavior change (e.g. numpy.bool_ failing isinstance(bool)) is diagnosable instead of invisible. - Add regression tests asserting 0 and empty string (falsy non-bool) also fail closed, making the full scope of the behavior change explicit. Addresses feedback from mahirhir and tonydzi on PR 7012. --- src/google/adk/tools/function_tool.py | 16 +++++++ src/google/adk/tools/mcp_tool/mcp_tool.py | 16 +++++++ .../unittests/tools/mcp_tool/test_mcp_tool.py | 45 +++++++++++++++++++ tests/unittests/tools/test_function_tool.py | 38 ++++++++++++++++ 4 files changed, 115 insertions(+) diff --git a/src/google/adk/tools/function_tool.py b/src/google/adk/tools/function_tool.py index 0dfb0dcb430..c16b1a6f0c6 100644 --- a/src/google/adk/tools/function_tool.py +++ b/src/google/adk/tools/function_tool.py @@ -202,8 +202,24 @@ async def check_require_confirmation( result = await self._invoke_callable( self._require_confirmation, args_to_call ) + if inspect.isawaitable(result): + logger.warning( + "require_confirmation predicate for tool '%s' returned an" + " un-awaited awaitable (%s); the predicate did not actually run." + " Treating this as requiring confirmation.", + self.name, + type(result).__name__, + ) + return True if isinstance(result, bool): return result + logger.warning( + "require_confirmation predicate for tool '%s' returned %r (%s)," + " which is not a bool. Treating this as requiring confirmation.", + self.name, + result, + type(result).__name__, + ) return True return bool(self._require_confirmation) diff --git a/src/google/adk/tools/mcp_tool/mcp_tool.py b/src/google/adk/tools/mcp_tool/mcp_tool.py index 7618d09fb2a..0fb8e109cd5 100644 --- a/src/google/adk/tools/mcp_tool/mcp_tool.py +++ b/src/google/adk/tools/mcp_tool/mcp_tool.py @@ -470,8 +470,24 @@ async def check_require_confirmation( result = await self._invoke_callable( self._require_confirmation, args_to_call ) + if inspect.isawaitable(result): + logger.warning( + "require_confirmation predicate for tool '%s' returned an" + " un-awaited awaitable (%s); the predicate did not actually run." + " Treating this as requiring confirmation.", + self.name, + type(result).__name__, + ) + return True if isinstance(result, bool): return result + logger.warning( + "require_confirmation predicate for tool '%s' returned %r (%s)," + " which is not a bool. Treating this as requiring confirmation.", + self.name, + result, + type(result).__name__, + ) return True return bool(self._require_confirmation) diff --git a/tests/unittests/tools/mcp_tool/test_mcp_tool.py b/tests/unittests/tools/mcp_tool/test_mcp_tool.py index dde78cb6f0a..dcb100b02e2 100644 --- a/tests/unittests/tools/mcp_tool/test_mcp_tool.py +++ b/tests/unittests/tools/mcp_tool/test_mcp_tool.py @@ -1296,6 +1296,51 @@ def returns_false(param1: str): ) assert result is False + + + @pytest.mark.asyncio + async def test_check_require_confirmation_callable_returns_zero( + self, + ): + """A predicate returning 0 (falsy non-bool) must require confirmation.""" + + def returns_zero(param1: str): + return 0 + + tool = MCPTool( + mcp_tool=self.mock_mcp_tool, + mcp_session_manager=self.mock_session_manager, + require_confirmation=returns_zero, + ) + tool_context = Mock(spec=ToolContext) + + result = await tool.check_require_confirmation( + {"param1": "test_value"}, tool_context + ) + + assert result is True + + @pytest.mark.asyncio + async def test_check_require_confirmation_callable_returns_empty_string( + self, + ): + """A predicate returning '' (falsy non-bool) must require confirmation.""" + + def returns_empty_string(param1: str): + return "" + + tool = MCPTool( + mcp_tool=self.mock_mcp_tool, + mcp_session_manager=self.mock_session_manager, + require_confirmation=returns_empty_string, + ) + tool_context = Mock(spec=ToolContext) + + result = await tool.check_require_confirmation( + {"param1": "test_value"}, tool_context + ) + + assert result is True def test_init_validation(self): """Test that initialization validates required parameters.""" diff --git a/tests/unittests/tools/test_function_tool.py b/tests/unittests/tools/test_function_tool.py index 1dc73520600..65d2fce045a 100644 --- a/tests/unittests/tools/test_function_tool.py +++ b/tests/unittests/tools/test_function_tool.py @@ -417,6 +417,44 @@ def returns_false(arg1: str): assert result is False +@pytest.mark.asyncio +async def test_check_require_confirmation_callable_returns_zero( + mock_tool_context, +): + """A predicate returning 0 (falsy non-bool) must require confirmation.""" + + def returns_zero(arg1: str): + return 0 + + tool = FunctionTool( + lambda arg1: {"received_arg": arg1}, + require_confirmation=returns_zero, + ) + result = await tool.check_require_confirmation( + {"arg1": "hello"}, mock_tool_context + ) + assert result is True + + +@pytest.mark.asyncio +async def test_check_require_confirmation_callable_returns_empty_string( + mock_tool_context, +): + """A predicate returning '' (falsy non-bool) must require confirmation.""" + + def returns_empty_string(arg1: str): + return "" + + tool = FunctionTool( + lambda arg1: {"received_arg": arg1}, + require_confirmation=returns_empty_string, + ) + result = await tool.check_require_confirmation( + {"arg1": "hello"}, mock_tool_context + ) + assert result is True + + @pytest.mark.asyncio async def test_run_async_with_tool_context_and_unexpected_argument(): """Test that run_async handles tool_context and filters out unexpected arguments.""" From df92611fd7f072e042a6489642a0045d0998da76 Mon Sep 17 00:00:00 2001 From: boopathi-376 Date: Mon, 7 Sep 2026 10:09:30 +0530 Subject: [PATCH 3/3] docs(tools): document non-bool require_confirmation behavior Update require_confirmation docstrings in FunctionTool, McpTool, and McpToolset to state that a non-bool return (including None) is treated as requiring confirmation, matching the runtime behavior fixed in the prior commits. --- src/google/adk/tools/function_tool.py | 4 +++- src/google/adk/tools/mcp_tool/mcp_tool.py | 11 +++++++---- src/google/adk/tools/mcp_tool/mcp_toolset.py | 6 ++++-- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/google/adk/tools/function_tool.py b/src/google/adk/tools/function_tool.py index c16b1a6f0c6..2a567aac296 100644 --- a/src/google/adk/tools/function_tool.py +++ b/src/google/adk/tools/function_tool.py @@ -110,7 +110,9 @@ def __init__( require_confirmation: Whether this tool requires confirmation. A boolean or a callable that takes the function's arguments and returns a boolean. If the callable returns True, the tool will require confirmation from the - user. + user. Any return value that is not a bool (including None, e.g. from a + function that falls through without an explicit return, or an + un-awaited awaitable) is treated as requiring confirmation. """ self._spec = CallableSpec(func) name = _function_tool_declarations.get_callable_name(func) diff --git a/src/google/adk/tools/mcp_tool/mcp_tool.py b/src/google/adk/tools/mcp_tool/mcp_tool.py index 0fb8e109cd5..5464112757e 100644 --- a/src/google/adk/tools/mcp_tool/mcp_tool.py +++ b/src/google/adk/tools/mcp_tool/mcp_tool.py @@ -302,10 +302,13 @@ def __init__( mcp_session_manager: The MCP session manager to use for communication. auth_scheme: The authentication scheme to use. auth_credential: The authentication credential to use. - require_confirmation: Whether this tool requires confirmation. A boolean - or a callable that takes the function's arguments and returns a - boolean. If the callable returns True, the tool will require - confirmation from the user. + func: The function to wrap. + require_confirmation: Whether this tool requires confirmation. A boolean or + a callable that takes the function's arguments and returns a boolean. If + the callable returns True, the tool will require confirmation from the + user. Any return value that is not a bool (including None, e.g. from a + function that falls through without an explicit return, or an + un-awaited awaitable) is treated as requiring confirmation. header_provider: Optional function to provide dynamic headers. progress_callback: Optional callback to receive progress notifications from MCP server during long-running tool execution. Can be either: diff --git a/src/google/adk/tools/mcp_tool/mcp_toolset.py b/src/google/adk/tools/mcp_tool/mcp_toolset.py index dcf4a7e0c68..a386c3a40a1 100644 --- a/src/google/adk/tools/mcp_tool/mcp_toolset.py +++ b/src/google/adk/tools/mcp_tool/mcp_toolset.py @@ -199,8 +199,10 @@ def __init__( errlog: TextIO stream for error logging. auth_scheme: The auth scheme of the tool for tool calling auth_credential: The auth credential of the tool for tool calling - require_confirmation: Whether tools in this toolset require confirmation. - Can be a single boolean or a callable to apply to all tools. + require_confirmation: Whether tools in this toolset require confirmation. + Can be a single boolean or a callable to apply to all tools. Forwarded + as-is to each McpTool this toolset builds (see McpTool.require_confirmation + for how a non-bool callable return is handled). header_provider: A callable that takes a ReadonlyContext and returns a dictionary of headers to be used for the MCP session. progress_callback: Optional callback to receive progress notifications