From bf104e49ad3ec7e80e515eeebbf9b39a05a92856 Mon Sep 17 00:00:00 2001 From: Sourabh Sarupria Date: Tue, 20 May 2025 16:19:07 -0700 Subject: [PATCH 1/5] Fix type annotations and parameter handling in Agent class 1. Type Annotation Improvements: - Changed kwargs: Any to kwargs: Dict[str, Any] in _run_loop method - Changed dict[str, Any] to Dict[str, Any] in _execute_event_loop_cycle - Improved all Callable type annotations to Callable[..., Any] 2. Default Parameter Value Fix: - Changed callback_handler default from PrintingCallbackHandler() to None - This avoids the anti-pattern of using a mutable default parameter value 3. Parameter Mutation Protection: - Added .copy() to kwargs in both __call__ and stream_async methods - This prevents modifications to the kwargs dictionary from affecting the original --- src/strands/agent/agent.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/strands/agent/agent.py b/src/strands/agent/agent.py index bfa83fe20..310948460 100644 --- a/src/strands/agent/agent.py +++ b/src/strands/agent/agent.py @@ -70,7 +70,7 @@ def __init__(self, agent: "Agent") -> None: # agent tools and thus break their execution. self._agent = agent - def __getattr__(self, name: str) -> Callable: + def __getattr__(self, name: str) -> Callable[..., Any]: """Call tool as a function. This method enables the method-style interface (e.g., `agent.tool.tool_name(param="value")`). @@ -177,7 +177,7 @@ def __init__( messages: Optional[Messages] = None, tools: Optional[List[Union[str, Dict[str, str], Any]]] = None, system_prompt: Optional[str] = None, - callback_handler: Optional[Callable] = PrintingCallbackHandler(), + callback_handler: Optional[Callable[..., Any]] = None, conversation_manager: Optional[ConversationManager] = None, max_parallel_tools: int = os.cpu_count() or 1, record_direct_tool_call: bool = True, @@ -332,7 +332,7 @@ def __call__(self, prompt: str, **kwargs: Any) -> AgentResult: try: # Run the event loop and get the result - result = self._run_loop(prompt, kwargs) + result = self._run_loop(prompt, kwargs.copy()) self._end_agent_trace_span(response=result) @@ -415,7 +415,7 @@ def target_callback() -> None: thread.join() def _run_loop( - self, prompt: str, kwargs: Any, supplementary_callback_handler: Optional[Callable] = None + self, prompt: str, kwargs: Dict[str, Any], supplementary_callback_handler: Optional[Callable[..., Any]] = None ) -> AgentResult: """Execute the agent's event loop with the given prompt and parameters.""" try: @@ -441,7 +441,7 @@ def _run_loop( finally: self.conversation_manager.apply_management(self) - def _execute_event_loop_cycle(self, callback_handler: Callable, kwargs: dict[str, Any]) -> AgentResult: + def _execute_event_loop_cycle(self, callback_handler: Callable[..., Any], kwargs: Dict[str, Any]) -> AgentResult: """Execute the event loop cycle with retry logic for context window limits. This internal method handles the execution of the event loop cycle and implements From 7ef88c0f04aa4fe1361dfb5537ea9314cb5a5bbb Mon Sep 17 00:00:00 2001 From: Sourabh Sarupria Date: Mon, 26 May 2025 18:41:45 -0700 Subject: [PATCH 2/5] Implement sentinel object pattern for callback_handler default value This addresses PR feedback by: 1. Adding a sentinel object _DEFAULT_CALLBACK_HANDLER to distinguish between default and explicit None 2. Using the sentinel as the default parameter value for callback_handler 3. Creating a new PrintingCallbackHandler instance when the sentinel is used 4. Preserving the behavior where explicitly passing None uses null_callback_handler 5. Updating the docstring to reflect these changes --- src/strands/agent/agent.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/strands/agent/agent.py b/src/strands/agent/agent.py index 310948460..e7f60d774 100644 --- a/src/strands/agent/agent.py +++ b/src/strands/agent/agent.py @@ -43,6 +43,9 @@ logger = logging.getLogger(__name__) +# Sentinel object to distinguish between explicit None and default parameter value +_DEFAULT_CALLBACK_HANDLER = object() + class Agent: """Core Agent interface. @@ -177,7 +180,7 @@ def __init__( messages: Optional[Messages] = None, tools: Optional[List[Union[str, Dict[str, str], Any]]] = None, system_prompt: Optional[str] = None, - callback_handler: Optional[Callable[..., Any]] = None, + callback_handler: Optional[Callable[..., Any]] = _DEFAULT_CALLBACK_HANDLER, conversation_manager: Optional[ConversationManager] = None, max_parallel_tools: int = os.cpu_count() or 1, record_direct_tool_call: bool = True, @@ -204,7 +207,8 @@ def __init__( system_prompt: System prompt to guide model behavior. If None, the model will behave according to its default settings. callback_handler: Callback for processing events as they happen during agent execution. - Defaults to strands.handlers.PrintingCallbackHandler if None. + If not provided (using the default), a new PrintingCallbackHandler instance is created. + If explicitly set to None, null_callback_handler is used. conversation_manager: Manager for conversation history and context window. Defaults to strands.agent.conversation_manager.SlidingWindowConversationManager if None. max_parallel_tools: Maximum number of tools to run in parallel when the model returns multiple tool calls. @@ -222,7 +226,13 @@ def __init__( self.messages = messages if messages is not None else [] self.system_prompt = system_prompt - self.callback_handler = callback_handler or null_callback_handler + + # If the default sentinel is used, create a new PrintingCallbackHandler instance + if callback_handler is _DEFAULT_CALLBACK_HANDLER: + self.callback_handler = PrintingCallbackHandler() + else: + # Otherwise use the provided handler or null_callback_handler if None was explicitly passed + self.callback_handler = callback_handler or null_callback_handler self.conversation_manager = conversation_manager if conversation_manager else SlidingWindowConversationManager() From a772d39a6e671d0e1f0c5a06009cb2efb1345892 Mon Sep 17 00:00:00 2001 From: Arron Bailiss Date: Tue, 3 Jun 2025 10:29:37 -0400 Subject: [PATCH 3/5] fix: lint --- src/strands/agent/agent.py | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/strands/agent/agent.py b/src/strands/agent/agent.py index e7f60d774..40df1a69e 100644 --- a/src/strands/agent/agent.py +++ b/src/strands/agent/agent.py @@ -43,8 +43,15 @@ logger = logging.getLogger(__name__) -# Sentinel object to distinguish between explicit None and default parameter value -_DEFAULT_CALLBACK_HANDLER = object() + +# Sentinel class and object to distinguish between explicit None and default parameter value +class _DefaultCallbackHandlerSentinel: + """Sentinel class to distinguish between explicit None and default parameter value.""" + + pass + + +_DEFAULT_CALLBACK_HANDLER = _DefaultCallbackHandlerSentinel() class Agent: @@ -180,7 +187,9 @@ def __init__( messages: Optional[Messages] = None, tools: Optional[List[Union[str, Dict[str, str], Any]]] = None, system_prompt: Optional[str] = None, - callback_handler: Optional[Callable[..., Any]] = _DEFAULT_CALLBACK_HANDLER, + callback_handler: Optional[ + Union[Callable[..., Any], _DefaultCallbackHandlerSentinel] + ] = _DEFAULT_CALLBACK_HANDLER, conversation_manager: Optional[ConversationManager] = None, max_parallel_tools: int = os.cpu_count() or 1, record_direct_tool_call: bool = True, @@ -226,13 +235,17 @@ def __init__( self.messages = messages if messages is not None else [] self.system_prompt = system_prompt - - # If the default sentinel is used, create a new PrintingCallbackHandler instance - if callback_handler is _DEFAULT_CALLBACK_HANDLER: + + # If not provided, create a new PrintingCallbackHandler instance + # If explicitly set to None, use null_callback_handler + # Otherwise use the passed callback_handler + self.callback_handler: Union[Callable[..., Any], PrintingCallbackHandler] + if isinstance(callback_handler, _DefaultCallbackHandlerSentinel): self.callback_handler = PrintingCallbackHandler() + elif callback_handler is None: + self.callback_handler = null_callback_handler else: - # Otherwise use the provided handler or null_callback_handler if None was explicitly passed - self.callback_handler = callback_handler or null_callback_handler + self.callback_handler = callback_handler self.conversation_manager = conversation_manager if conversation_manager else SlidingWindowConversationManager() From b9de670a2c4b6b596ef36c66cf1c31af13796091 Mon Sep 17 00:00:00 2001 From: Arron Bailiss Date: Tue, 3 Jun 2025 10:55:35 -0400 Subject: [PATCH 4/5] test: add unit tests for Agent.callback_handler initialization --- tests/strands/agent/test_agent.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/strands/agent/test_agent.py b/tests/strands/agent/test_agent.py index 4a63fa31f..0ea20b642 100644 --- a/tests/strands/agent/test_agent.py +++ b/tests/strands/agent/test_agent.py @@ -686,6 +686,37 @@ def test_agent_with_callback_handler_none_uses_null_handler(): assert agent.callback_handler == null_callback_handler +def test_agent_callback_handler_not_provided_creates_new_instances(): + """Test that when callback_handler is not provided, new PrintingCallbackHandler instances are created.""" + # Create two agents without providing callback_handler + agent1 = Agent() + agent2 = Agent() + + # Both should have PrintingCallbackHandler instances + assert isinstance(agent1.callback_handler, PrintingCallbackHandler) + assert isinstance(agent2.callback_handler, PrintingCallbackHandler) + + # But they should be different object instances + assert agent1.callback_handler is not agent2.callback_handler + + +def test_agent_callback_handler_explicit_none_uses_null_handler(): + """Test that when callback_handler is explicitly set to None, null_callback_handler is used.""" + agent = Agent(callback_handler=None) + + # Should use null_callback_handler + assert agent.callback_handler is null_callback_handler + + +def test_agent_callback_handler_custom_handler_used(): + """Test that when a custom callback_handler is provided, it is used.""" + custom_handler = unittest.mock.Mock() + agent = Agent(callback_handler=custom_handler) + + # Should use the provided custom handler + assert agent.callback_handler is custom_handler + + @pytest.mark.asyncio async def test_stream_async_returns_all_events(mock_event_loop_cycle): agent = Agent() From be9d1cacc5a6af3a34a6fd83403f644ea45cf644 Mon Sep 17 00:00:00 2001 From: Arron Bailiss Date: Tue, 3 Jun 2025 11:01:20 -0400 Subject: [PATCH 5/5] fix: use original kwargs instead of copying --- src/strands/agent/agent.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/strands/agent/agent.py b/src/strands/agent/agent.py index 40df1a69e..0651d4521 100644 --- a/src/strands/agent/agent.py +++ b/src/strands/agent/agent.py @@ -355,7 +355,7 @@ def __call__(self, prompt: str, **kwargs: Any) -> AgentResult: try: # Run the event loop and get the result - result = self._run_loop(prompt, kwargs.copy()) + result = self._run_loop(prompt, kwargs) self._end_agent_trace_span(response=result)