fix(graph): prevent reset_executor_state from corrupting MultiAgentBase state#1988
fix(graph): prevent reset_executor_state from corrupting MultiAgentBase state#1988giulio-leone wants to merge 2 commits intostrands-agents:mainfrom
Conversation
…se state GraphNode.reset_executor_state() checked hasattr(self.executor, 'state') but did not verify the state type before overwriting it with AgentState. When the executor is a MultiAgentBase (e.g. a nested Graph), its state is a GraphState, and overwriting it with AgentState corrupts the executor. The __post_init__ method already had the correct guard: hasattr(self.executor.state, 'get') but reset_executor_state() was missing it. This affected two call sites: - _execute_node (when reset_on_revisit is enabled with nested graphs) - deserialize_state (unconditionally resets all nodes on completed runs) Fixes strands-agents#1775
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/strands review |
|
Assessment: Approve Clean, well-scoped bugfix that correctly mirrors the existing Review Details
Nice fix — minimal and well-targeted. 👍 |
| self.executor.messages = copy.deepcopy(self._initial_messages) | ||
|
|
||
| if hasattr(self.executor, "state"): | ||
| if hasattr(self.executor, "state") and hasattr(self.executor.state, "get"): |
There was a problem hiding this comment.
Suggestion (non-blocking): The hasattr(self.executor.state, "get") guard relies on duck typing to distinguish AgentState from GraphState. This works today because GraphState is a dataclass without a get() method, but it would silently break if GraphState ever gains one.
A more explicit guard — such as isinstance(self.executor.state, AgentState) or not isinstance(self.executor, MultiAgentBase) — would make the intent clearer and be more resilient to future changes.
Since __post_init__ uses the same pattern, this could be addressed as a follow-up to improve both sites together.
There was a problem hiding this comment.
+1 on this actually, can we update the condition? This opens the door for a possible bug later on
Summary
Fixes #1775 —
GraphNode.reset_executor_state()corruptsMultiAgentBaseexecutor state by overwritingGraphStatewithAgentState.Root Cause
reset_executor_state()checkshasattr(self.executor, 'state')without verifying the state type:__post_init__already had the correct guard (hasattr(self.executor.state, 'get')), butreset_executor_state()was missing it.Impact
Two call sites affected:
_execute_nodereset_on_revisit=Truewith nested graph in a cycledeserialize_stateFix
Mirror the
__post_init__guard:Test
Added
test_reset_executor_state_preserves_graph_state_for_nested_graph— creates aGraphNodewith a nestedGraphexecutor and verifiesreset_executor_state()preserves theGraphStatetype.All 49 graph tests pass.