From 0ae478ece258509187ded79130dd6f4a290ab63f Mon Sep 17 00:00:00 2001 From: gongzihao Date: Mon, 24 Aug 2026 22:07:13 +0800 Subject: [PATCH 1/2] fix: incorrectly removes ActionEvent from view --- .../openhands/sdk/context/view/view.py | 31 ++++++++++++++++--- .../openhands/sdk/conversation/state.py | 19 ++++++++++-- .../sdk/conversation/test_state_view_cache.py | 8 ++--- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/openhands-sdk/openhands/sdk/context/view/view.py b/openhands-sdk/openhands/sdk/context/view/view.py index ad9838ab6e..97516fbb9a 100644 --- a/openhands-sdk/openhands/sdk/context/view/view.py +++ b/openhands-sdk/openhands/sdk/context/view/view.py @@ -7,13 +7,16 @@ from pydantic import BaseModel, Field from openhands.sdk.context.view.manipulation_indices import ManipulationIndices -from openhands.sdk.context.view.properties import ALL_PROPERTIES +from openhands.sdk.context.view.properties import ( + ALL_PROPERTIES, + ToolCallMatchingProperty, +) from openhands.sdk.event import ( Condensation, CondensationRequest, LLMConvertibleEvent, ) -from openhands.sdk.event.base import Event +from openhands.sdk.event.base import Event, EventID logger = getLogger(__name__) @@ -74,6 +77,8 @@ def __getitem__( def enforce_properties( self, all_events: Sequence[Event], + *, + allowed_unmatched_action_ids: set[EventID] | None = None, ) -> None: """Enforce all properties on the list of current view events. @@ -84,10 +89,16 @@ def enforce_properties( properties via the associated manipulation indices, any time a property must be enforced a warning is logged. + ``allowed_unmatched_action_ids`` identifies action events that may appear + without matching observations, such as actions awaiting user confirmation. + Modifies the view in-place. """ + allowed_unmatched_action_ids = allowed_unmatched_action_ids or set() for property in ALL_PROPERTIES: events_to_forget = property.enforce(self.events, all_events) + if isinstance(property, ToolCallMatchingProperty): + events_to_forget -= allowed_unmatched_action_ids if events_to_forget: logger.warning( f"Property {property.__class__} enforced, " @@ -106,7 +117,10 @@ def enforce_properties( # If we did hit a break in the loop, a property applied and now we need to check # all the properties again to see if any are unblocked. - self.enforce_properties(all_events) + self.enforce_properties( + all_events, + allowed_unmatched_action_ids=allowed_unmatched_action_ids, + ) def append_event(self, event: Event) -> None: """Append an event to the end of the view, applying any condensation semantics @@ -140,7 +154,11 @@ def append_event(self, event: Event) -> None: ) @staticmethod - def from_events(events: Sequence[Event]) -> View: + def from_events( + events: Sequence[Event], + *, + allowed_unmatched_action_ids: set[EventID] | None = None, + ) -> View: """Create a view from a list of events, respecting the semantics of any condensation events. """ @@ -155,6 +173,9 @@ def from_events(events: Sequence[Event]) -> View: # Once all the events are loaded enforce the relevant properties to ensure # the construction was done properly. - result.enforce_properties(events) + result.enforce_properties( + events, + allowed_unmatched_action_ids=allowed_unmatched_action_ids, + ) return result diff --git a/openhands-sdk/openhands/sdk/conversation/state.py b/openhands-sdk/openhands/sdk/conversation/state.py index 9254a0d829..ee28aab1f3 100644 --- a/openhands-sdk/openhands/sdk/conversation/state.py +++ b/openhands-sdk/openhands/sdk/conversation/state.py @@ -301,6 +301,14 @@ def active_branch(self, limit: int | None = None) -> list[Event]: """ return self._events.path_to_root(self._resolve_active_leaf(), limit=limit) + def _allowed_unmatched_action_ids(self, branch: Sequence[Event]) -> set[EventID]: + if ( + self.execution_status + != ConversationExecutionStatus.WAITING_FOR_CONFIRMATION + ): + return set() + return {action.id for action in self.get_unmatched_actions(branch)} + def _stamp_parent_id(self, event: Event) -> Event: """Return ``event`` with ``parent_id`` set to the active leaf if unset.""" if event.parent_id is not None: @@ -376,7 +384,11 @@ def view(self) -> View: # Diverged branch (navigation/fork), first populate, or recovery # from the failure above → full rebuild from the active branch. - self._view = View.from_events(self._events.path_to_root(leaf)) + branch = self._events.path_to_root(leaf) + self._view = View.from_events( + branch, + allowed_unmatched_action_ids=self._allowed_unmatched_action_ids(branch), + ) self._view_branch_leaf = leaf return self._view @@ -391,7 +403,10 @@ def rebuild_view(self) -> None: with self._view_lock: leaf = self._resolve_active_leaf() branch = self._events.path_to_root(leaf) - self._view = View.from_events(branch) + self._view = View.from_events( + branch, + allowed_unmatched_action_ids=self._allowed_unmatched_action_ids(branch), + ) self._view_branch_leaf = leaf @property diff --git a/tests/sdk/conversation/test_state_view_cache.py b/tests/sdk/conversation/test_state_view_cache.py index 483b917ce4..5e1710f841 100644 --- a/tests/sdk/conversation/test_state_view_cache.py +++ b/tests/sdk/conversation/test_state_view_cache.py @@ -117,10 +117,10 @@ def test_hot_path_does_not_call_enforce_properties(state): call_count = 0 original = View.enforce_properties - def counting_enforce(self, all_events): + def counting_enforce(self, all_events, **kwargs): nonlocal call_count call_count += 1 - return original(self, all_events) + return original(self, all_events, **kwargs) with patch.object(View, "enforce_properties", counting_enforce): for i in range(10): @@ -136,10 +136,10 @@ def test_rebuild_view_runs_enforce_properties(state): call_count = 0 original = View.enforce_properties - def counting_enforce(self, all_events): + def counting_enforce(self, all_events, **kwargs): nonlocal call_count call_count += 1 - return original(self, all_events) + return original(self, all_events, **kwargs) with patch.object(View, "enforce_properties", counting_enforce): state.rebuild_view() From bef1c7657cfa19f0049469a0be63981f9f84d537 Mon Sep 17 00:00:00 2001 From: gongzihao Date: Mon, 24 Aug 2026 22:15:53 +0800 Subject: [PATCH 2/2] add: test_confirmation_mode.py --- .../local/test_confirmation_mode.py | 64 ++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/tests/sdk/conversation/local/test_confirmation_mode.py b/tests/sdk/conversation/local/test_confirmation_mode.py index 4d84f56226..dcd9140858 100644 --- a/tests/sdk/conversation/local/test_confirmation_mode.py +++ b/tests/sdk/conversation/local/test_confirmation_mode.py @@ -4,8 +4,9 @@ Tests the core behavior: pause action execution for user confirmation. """ +import uuid from collections.abc import Sequence -from typing import ClassVar +from typing import Any, ClassVar from unittest.mock import MagicMock, Mock, patch import pytest @@ -650,6 +651,67 @@ def test_pause_during_confirmation_preserves_waiting_status(self): == ConversationExecutionStatus.PAUSED ) + def test_resume_confirmation_preserves_tool_call_message(self, tmp_path): + conversation_id = uuid.uuid4() + sent_messages: list[list[dict[str, Any]]] = [] + responses = [ + self._mock_action_once().return_value, + self._mock_message_only("Task completed successfully!").return_value, + ] + + def capture_completion(**kwargs): + sent_messages.append(kwargs["messages"]) + return responses.pop(0) + + def build_conversation(): + conversation = Conversation( + agent=self.agent, + workspace=tmp_path / "workspace", + persistence_dir=tmp_path / "state", + conversation_id=conversation_id, + visualizer=None, + delete_on_close=False, + ) + conversation.set_confirmation_policy(AlwaysConfirm()) + return conversation + + with patch( + "openhands.sdk.llm.llm.litellm_completion", + side_effect=capture_completion, + ): + conversation = build_conversation() + conversation.send_message( + Message(role="user", content=[TextContent(text="execute a command")]) + ) + conversation.run() + assert ( + conversation.state.execution_status + == ConversationExecutionStatus.WAITING_FOR_CONFIRMATION + ) + + conversation.close() + conversation = build_conversation() + assert ( + conversation.state.execution_status + == ConversationExecutionStatus.WAITING_FOR_CONFIRMATION + ) + + conversation.run() + conversation.close() + + assert len(sent_messages) == 2 + last_messages = sent_messages[-1] + tool_message_index = next( + i + for i, message in enumerate(last_messages) + if message.get("role") == "tool" and message.get("tool_call_id") == "call_1" + ) + preceding_message = last_messages[tool_message_index - 1] + tool_calls = preceding_message.get("tool_calls") or [] + + assert preceding_message["role"] == "assistant" + assert [tool_call["id"] for tool_call in tool_calls] == ["call_1"] + def test_is_confirmation_mode_active_property(self): """Test the is_confirmation_mode_active property behavior.""" # Initially, no security analyzer and NeverConfirm policy