diff --git a/src/google/adk/tools/function_tool.py b/src/google/adk/tools/function_tool.py index 266a97c2d62..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) @@ -199,10 +201,28 @@ 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 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) 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..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: @@ -467,10 +470,28 @@ 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 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) @override 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 diff --git a/tests/unittests/tools/mcp_tool/test_mcp_tool.py b/tests/unittests/tools/mcp_tool/test_mcp_tool.py index 245b22d2d61..dcb100b02e2 100644 --- a/tests/unittests/tools/mcp_tool/test_mcp_tool.py +++ b/tests/unittests/tools/mcp_tool/test_mcp_tool.py @@ -1221,6 +1221,127 @@ 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 + + + @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.""" # This test ensures that the MCPTool properly handles its dependencies @@ -2365,4 +2486,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..65d2fce045a 100644 --- a/tests/unittests/tools/test_function_tool.py +++ b/tests/unittests/tools/test_function_tool.py @@ -350,6 +350,111 @@ 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_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.""" @@ -755,4 +860,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