From c6c6c202f65c638bc7dd9e04f2b0c21f8a69cadd Mon Sep 17 00:00:00 2001 From: Graham Neubig Date: Mon, 7 Jul 2025 23:33:57 +0200 Subject: [PATCH] Fix CLI thought display order issue (#9417) Co-authored-by: openhands --- openhands/cli/tui.py | 41 ++++- tests/unit/test_cli_thought_order.py | 240 +++++++++++++++++++++++++++ 2 files changed, 272 insertions(+), 9 deletions(-) create mode 100644 tests/unit/test_cli_thought_order.py diff --git a/openhands/cli/tui.py b/openhands/cli/tui.py index 17dbeb5017..27b703a4fe 100644 --- a/openhands/cli/tui.py +++ b/openhands/cli/tui.py @@ -53,6 +53,10 @@ ENABLE_STREAMING = False # FIXME: this doesn't work # Global TextArea for streaming output streaming_output_text_area: TextArea | None = None +# Track recent thoughts to prevent duplicate display +recent_thoughts: list[str] = [] +MAX_RECENT_THOUGHTS = 5 + # Color and styling constants COLOR_GOLD = '#FFD700' COLOR_GREY = '#808080' @@ -183,19 +187,27 @@ def display_initial_user_prompt(prompt: str) -> None: # Prompt output display functions +def display_thought_if_new(thought: str) -> None: + """Display a thought only if it hasn't been displayed recently.""" + global recent_thoughts + if thought and thought.strip(): + # Check if this thought was recently displayed + if thought not in recent_thoughts: + display_message(thought) + recent_thoughts.append(thought) + # Keep only the most recent thoughts + if len(recent_thoughts) > MAX_RECENT_THOUGHTS: + recent_thoughts.pop(0) + + def display_event(event: Event, config: OpenHandsConfig) -> None: global streaming_output_text_area with print_lock: - if isinstance(event, Action): - if hasattr(event, 'thought'): - display_message(event.thought) - if hasattr(event, 'final_thought'): - display_message(event.final_thought) - if isinstance(event, MessageAction): - if event.source == EventSource.AGENT: - display_message(event.content) - if isinstance(event, CmdRunAction): + # For CmdRunAction, display thought first, then command + if hasattr(event, 'thought') and event.thought: + display_message(event.thought) + # Only display the command if it's not already confirmed # Commands are always shown when AWAITING_CONFIRMATION, so we don't need to show them again when CONFIRMED if event.confirmation_state != ActionConfirmationStatus.CONFIRMED: @@ -203,6 +215,17 @@ def display_event(event: Event, config: OpenHandsConfig) -> None: if event.confirmation_state == ActionConfirmationStatus.CONFIRMED: initialize_streaming_output() + elif isinstance(event, Action): + # For other actions, display thoughts normally + if hasattr(event, 'thought') and event.thought: + display_message(event.thought) + if hasattr(event, 'final_thought') and event.final_thought: + display_message(event.final_thought) + + if isinstance(event, MessageAction): + if event.source == EventSource.AGENT: + # Check if this message content is a duplicate thought + display_thought_if_new(event.content) elif isinstance(event, CmdOutputObservation): display_command_output(event.content) elif isinstance(event, FileEditObservation): diff --git a/tests/unit/test_cli_thought_order.py b/tests/unit/test_cli_thought_order.py new file mode 100644 index 0000000000..11e909b825 --- /dev/null +++ b/tests/unit/test_cli_thought_order.py @@ -0,0 +1,240 @@ +""" +Tests for CLI thought display order fix. +This ensures that agent thoughts are displayed before commands, not after. +""" + +from unittest.mock import MagicMock, patch + +from openhands.cli.tui import display_event +from openhands.core.config import OpenHandsConfig +from openhands.events import EventSource +from openhands.events.action import Action, ActionConfirmationStatus, CmdRunAction +from openhands.events.action.message import MessageAction + + +class TestThoughtDisplayOrder: + """Test that thoughts are displayed in the correct order relative to commands.""" + + @patch('openhands.cli.tui.display_message') + @patch('openhands.cli.tui.display_command') + def test_cmd_run_action_thought_before_command( + self, mock_display_command, mock_display_message + ): + """Test that for CmdRunAction, thought is displayed before command.""" + config = MagicMock(spec=OpenHandsConfig) + + # Create a CmdRunAction with a thought awaiting confirmation + cmd_action = CmdRunAction( + command='npm install', + thought='I need to install the dependencies first before running the tests.', + ) + cmd_action.confirmation_state = ActionConfirmationStatus.AWAITING_CONFIRMATION + + display_event(cmd_action, config) + + # Verify that display_message (for thought) was called before display_command + mock_display_message.assert_called_once_with( + 'I need to install the dependencies first before running the tests.' + ) + mock_display_command.assert_called_once_with(cmd_action) + + # Check the call order by examining the mock call history + all_calls = [] + all_calls.extend( + [('display_message', call) for call in mock_display_message.call_args_list] + ) + all_calls.extend( + [('display_command', call) for call in mock_display_command.call_args_list] + ) + + # Sort by the order they were called (this is a simplified check) + # In practice, we know display_message should be called first based on our code + assert mock_display_message.called + assert mock_display_command.called + + @patch('openhands.cli.tui.display_message') + @patch('openhands.cli.tui.display_command') + def test_cmd_run_action_no_thought( + self, mock_display_command, mock_display_message + ): + """Test that CmdRunAction without thought only displays command.""" + config = MagicMock(spec=OpenHandsConfig) + + # Create a CmdRunAction without a thought + cmd_action = CmdRunAction(command='npm install') + cmd_action.confirmation_state = ActionConfirmationStatus.AWAITING_CONFIRMATION + + display_event(cmd_action, config) + + # Verify that display_message was not called (no thought) + mock_display_message.assert_not_called() + mock_display_command.assert_called_once_with(cmd_action) + + @patch('openhands.cli.tui.display_message') + @patch('openhands.cli.tui.display_command') + def test_cmd_run_action_empty_thought( + self, mock_display_command, mock_display_message + ): + """Test that CmdRunAction with empty thought only displays command.""" + config = MagicMock(spec=OpenHandsConfig) + + # Create a CmdRunAction with empty thought + cmd_action = CmdRunAction(command='npm install', thought='') + cmd_action.confirmation_state = ActionConfirmationStatus.AWAITING_CONFIRMATION + + display_event(cmd_action, config) + + # Verify that display_message was not called (empty thought) + mock_display_message.assert_not_called() + mock_display_command.assert_called_once_with(cmd_action) + + @patch('openhands.cli.tui.display_message') + @patch('openhands.cli.tui.display_command') + @patch('openhands.cli.tui.initialize_streaming_output') + def test_cmd_run_action_confirmed_no_display( + self, mock_init_streaming, mock_display_command, mock_display_message + ): + """Test that confirmed CmdRunAction doesn't display command again but initializes streaming.""" + config = MagicMock(spec=OpenHandsConfig) + + # Create a confirmed CmdRunAction with thought + cmd_action = CmdRunAction( + command='npm install', + thought='I need to install the dependencies first before running the tests.', + ) + cmd_action.confirmation_state = ActionConfirmationStatus.CONFIRMED + + display_event(cmd_action, config) + + # Verify that thought is still displayed + mock_display_message.assert_called_once_with( + 'I need to install the dependencies first before running the tests.' + ) + # But command should not be displayed again (already shown when awaiting confirmation) + mock_display_command.assert_not_called() + # Streaming should be initialized + mock_init_streaming.assert_called_once() + + @patch('openhands.cli.tui.display_message') + def test_other_action_thought_display(self, mock_display_message): + """Test that other Action types still display thoughts normally.""" + config = MagicMock(spec=OpenHandsConfig) + + # Create a generic Action with thought + action = Action() + action.thought = 'This is a thought for a generic action.' + + display_event(action, config) + + # Verify that thought is displayed + mock_display_message.assert_called_once_with( + 'This is a thought for a generic action.' + ) + + @patch('openhands.cli.tui.display_message') + def test_other_action_final_thought_display(self, mock_display_message): + """Test that other Action types display final thoughts.""" + config = MagicMock(spec=OpenHandsConfig) + + # Create a generic Action with final thought + action = Action() + action.final_thought = 'This is a final thought.' + + display_event(action, config) + + # Verify that final thought is displayed + mock_display_message.assert_called_once_with('This is a final thought.') + + @patch('openhands.cli.tui.display_message') + def test_message_action_from_agent(self, mock_display_message): + """Test that MessageAction from agent is displayed.""" + config = MagicMock(spec=OpenHandsConfig) + + # Create a MessageAction from agent + message_action = MessageAction(content='Hello from agent') + message_action._source = EventSource.AGENT + + display_event(message_action, config) + + # Verify that message is displayed + mock_display_message.assert_called_once_with('Hello from agent') + + @patch('openhands.cli.tui.display_message') + def test_message_action_from_user_not_displayed(self, mock_display_message): + """Test that MessageAction from user is not displayed.""" + config = MagicMock(spec=OpenHandsConfig) + + # Create a MessageAction from user + message_action = MessageAction(content='Hello from user') + message_action._source = EventSource.USER + + display_event(message_action, config) + + # Verify that message is not displayed (only agent messages are shown) + mock_display_message.assert_not_called() + + @patch('openhands.cli.tui.display_message') + @patch('openhands.cli.tui.display_command') + def test_cmd_run_action_with_both_thoughts( + self, mock_display_command, mock_display_message + ): + """Test CmdRunAction with both thought and final_thought.""" + config = MagicMock(spec=OpenHandsConfig) + + # Create a CmdRunAction with both thoughts + cmd_action = CmdRunAction(command='npm install', thought='Initial thought') + cmd_action.final_thought = 'Final thought' + cmd_action.confirmation_state = ActionConfirmationStatus.AWAITING_CONFIRMATION + + display_event(cmd_action, config) + + # For CmdRunAction, only the regular thought should be displayed + # (final_thought is handled by the general Action case, but CmdRunAction is handled first) + mock_display_message.assert_called_once_with('Initial thought') + mock_display_command.assert_called_once_with(cmd_action) + + +class TestThoughtDisplayIntegration: + """Integration tests for the thought display order fix.""" + + def test_realistic_scenario_order(self): + """Test a realistic scenario to ensure proper order.""" + config = MagicMock(spec=OpenHandsConfig) + + # Track the order of calls + call_order = [] + + def track_display_message(message): + call_order.append(f'THOUGHT: {message}') + + def track_display_command(event): + call_order.append(f'COMMAND: {event.command}') + + with ( + patch( + 'openhands.cli.tui.display_message', side_effect=track_display_message + ), + patch( + 'openhands.cli.tui.display_command', side_effect=track_display_command + ), + ): + # Create the scenario from the issue + cmd_action = CmdRunAction( + command='npm install', + thought='I need to install the dependencies first before running the tests.', + ) + cmd_action.confirmation_state = ( + ActionConfirmationStatus.AWAITING_CONFIRMATION + ) + + display_event(cmd_action, config) + + # Verify the correct order + expected_order = [ + 'THOUGHT: I need to install the dependencies first before running the tests.', + 'COMMAND: npm install', + ] + + assert call_order == expected_order, ( + f'Expected {expected_order}, but got {call_order}' + )