From 9be673d5534ea60aa191a978fb29f9b2560d0f73 Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Thu, 30 Oct 2025 19:04:41 -0400 Subject: [PATCH] CLI: Create conversation last minute (#11576) Co-authored-by: openhands Co-authored-by: Engel Nyst --- openhands-cli/openhands_cli/agent_chat.py | 47 ++++-- openhands-cli/openhands_cli/setup.py | 84 ++++------ .../tests/commands/test_new_command.py | 71 +++++---- .../tests/commands/test_resume_command.py | 147 ++++++++++++++++++ openhands-cli/tests/test_confirmation_mode.py | 5 +- 5 files changed, 255 insertions(+), 99 deletions(-) create mode 100644 openhands-cli/tests/commands/test_resume_command.py diff --git a/openhands-cli/openhands_cli/agent_chat.py b/openhands-cli/openhands_cli/agent_chat.py index b7fcaaf359..882400e65e 100644 --- a/openhands-cli/openhands_cli/agent_chat.py +++ b/openhands-cli/openhands_cli/agent_chat.py @@ -6,6 +6,7 @@ Provides a conversation interface with an AI agent using OpenHands patterns. import sys from datetime import datetime +import uuid from openhands.sdk import ( Message, @@ -16,7 +17,11 @@ from prompt_toolkit import print_formatted_text from prompt_toolkit.formatted_text import HTML from openhands_cli.runner import ConversationRunner -from openhands_cli.setup import MissingAgentSpec, setup_conversation, start_fresh_conversation +from openhands_cli.setup import ( + MissingAgentSpec, + setup_conversation, + verify_agent_exists_or_setup_agent +) from openhands_cli.tui.settings.mcp_screen import MCPScreen from openhands_cli.tui.settings.settings_screen import SettingsScreen from openhands_cli.tui.status import display_status @@ -65,21 +70,33 @@ def run_cli_entry(resume_conversation_id: str | None = None) -> None: EOFError: If EOF is encountered """ + conversation_id = uuid.uuid4() + if resume_conversation_id: + try: + conversation_id = uuid.UUID(resume_conversation_id) + except ValueError as e: + print_formatted_text( + HTML( + f"Warning: '{resume_conversation_id}' is not a valid UUID." + ) + ) + return + try: - conversation = start_fresh_conversation(resume_conversation_id) + initialized_agent = verify_agent_exists_or_setup_agent() except MissingAgentSpec: print_formatted_text(HTML('\nSetup is required to use OpenHands CLI.')) print_formatted_text(HTML('\nGoodbye! 👋')) return - display_welcome(conversation.id, bool(resume_conversation_id)) + display_welcome(conversation_id, bool(resume_conversation_id)) # Track session start time for uptime calculation session_start_time = datetime.now() # Create conversation runner to handle state machine logic - runner = ConversationRunner(conversation) + runner = None session = get_session_prompter() # Main chat loop @@ -106,7 +123,7 @@ def run_cli_entry(resume_conversation_id: str | None = None) -> None: exit_confirmation = exit_session_confirmation() if exit_confirmation == UserConfirmation.ACCEPT: print_formatted_text(HTML('\nGoodbye! 👋')) - _print_exit_hint(conversation.id) + _print_exit_hint(conversation_id) break elif command == '/settings': @@ -116,19 +133,19 @@ def run_cli_entry(resume_conversation_id: str | None = None) -> None: elif command == '/mcp': mcp_screen = MCPScreen() - mcp_screen.display_mcp_info(conversation.agent) + mcp_screen.display_mcp_info(initialized_agent) continue elif command == '/clear': - display_welcome(conversation.id) + display_welcome(conversation_id) continue elif command == '/new': try: # Start a fresh conversation (no resume ID = new conversation) - conversation = setup_conversation() + conversation = setup_conversation(conversation_id) runner = ConversationRunner(conversation) - display_welcome(conversation.id, resume=False) + display_welcome(conversation_id, resume=False) print_formatted_text( HTML('✓ Started fresh conversation') ) @@ -158,6 +175,13 @@ def run_cli_entry(resume_conversation_id: str | None = None) -> None: continue elif command == '/resume': + if not runner: + print_formatted_text( + HTML('No active conversation running...') + ) + continue + + conversation = runner.conversation if not ( conversation.state.agent_status == AgentExecutionStatus.PAUSED or conversation.state.agent_status @@ -171,6 +195,9 @@ def run_cli_entry(resume_conversation_id: str | None = None) -> None: # Resume without new message message = None + if not runner: + conversation = setup_conversation(conversation_id) + runner = ConversationRunner(conversation) runner.process_message(message) print() # Add spacing @@ -179,7 +206,7 @@ def run_cli_entry(resume_conversation_id: str | None = None) -> None: exit_confirmation = exit_session_confirmation() if exit_confirmation == UserConfirmation.ACCEPT: print_formatted_text(HTML('\nGoodbye! 👋')) - _print_exit_hint(conversation.id) + _print_exit_hint(conversation_id) break # Clean up terminal state diff --git a/openhands-cli/openhands_cli/setup.py b/openhands-cli/openhands_cli/setup.py index 9e74fa99be..f6fb6cdb37 100644 --- a/openhands-cli/openhands_cli/setup.py +++ b/openhands-cli/openhands_cli/setup.py @@ -2,7 +2,7 @@ import uuid from prompt_toolkit import HTML, print_formatted_text -from openhands.sdk import BaseConversation, Conversation, Workspace, register_tool +from openhands.sdk import Agent, BaseConversation, Conversation, Workspace, register_tool from openhands.tools.execute_bash import BashTool from openhands.tools.file_editor import FileEditorTool from openhands.tools.task_tracker import TaskTrackerTool @@ -26,8 +26,38 @@ class MissingAgentSpec(Exception): pass -def setup_conversation( + +def load_agent_specs( conversation_id: str | None = None, +) -> Agent: + agent_store = AgentStore() + agent = agent_store.load(session_id=conversation_id) + if not agent: + raise MissingAgentSpec( + 'Agent specification not found. Please configure your agent settings.' + ) + return agent + + +def verify_agent_exists_or_setup_agent() -> Agent: + """Verify agent specs exists by attempting to load it. + + """ + settings_screen = SettingsScreen() + try: + agent = load_agent_specs() + return agent + except MissingAgentSpec: + # For first-time users, show the full settings flow with choice between basic/advanced + settings_screen.configure_settings(first_time=True) + + + # Try once again after settings setup attempt + return load_agent_specs() + + +def setup_conversation( + conversation_id: uuid, include_security_analyzer: bool = True ) -> BaseConversation: """ @@ -40,28 +70,8 @@ def setup_conversation( MissingAgentSpec: If agent specification is not found or invalid. """ - # Use provided conversation_id or generate a random one - if conversation_id is None: - conversation_id = uuid.uuid4() - elif isinstance(conversation_id, str): - try: - conversation_id = uuid.UUID(conversation_id) - except ValueError as e: - print_formatted_text( - HTML( - f"Warning: '{conversation_id}' is not a valid UUID." - ) - ) - raise e - with LoadingContext('Initializing OpenHands agent...'): - agent_store = AgentStore() - agent = agent_store.load(session_id=str(conversation_id)) - if not agent: - raise MissingAgentSpec( - 'Agent specification not found. Please configure your agent settings.' - ) - + agent = load_agent_specs(str(conversation_id)) if not include_security_analyzer: # Remove security analyzer from agent spec @@ -86,31 +96,3 @@ def setup_conversation( ) return conversation - - -def start_fresh_conversation( - resume_conversation_id: str | None = None -) -> BaseConversation: - """Start a fresh conversation by creating a new conversation instance. - - Handles the complete conversation setup process including settings screen - if agent configuration is missing. - - Args: - resume_conversation_id: Optional conversation ID to resume - - Returns: - BaseConversation: A new conversation instance - """ - conversation = None - settings_screen = SettingsScreen() - try: - conversation = setup_conversation(resume_conversation_id) - return conversation - except MissingAgentSpec: - # For first-time users, show the full settings flow with choice between basic/advanced - settings_screen.configure_settings(first_time=True) - - - # Try once again after settings setup attempt - return setup_conversation(resume_conversation_id) diff --git a/openhands-cli/tests/commands/test_new_command.py b/openhands-cli/tests/commands/test_new_command.py index 8b6d10249e..759c4b4918 100644 --- a/openhands-cli/tests/commands/test_new_command.py +++ b/openhands-cli/tests/commands/test_new_command.py @@ -4,51 +4,49 @@ from unittest.mock import MagicMock, patch from uuid import UUID from prompt_toolkit.input.defaults import create_pipe_input from prompt_toolkit.output.defaults import DummyOutput -from openhands_cli.setup import MissingAgentSpec, start_fresh_conversation +from openhands_cli.setup import MissingAgentSpec, verify_agent_exists_or_setup_agent, setup_conversation from openhands_cli.user_actions import UserConfirmation -@patch('openhands_cli.setup.setup_conversation') -def test_start_fresh_conversation_success(mock_setup_conversation): - """Test that start_fresh_conversation creates a new conversation successfully.""" - # Mock the conversation object - mock_conversation = MagicMock() - mock_conversation.id = UUID('12345678-1234-5678-9abc-123456789abc') - mock_setup_conversation.return_value = mock_conversation +@patch('openhands_cli.setup.load_agent_specs') +def test_verify_agent_exists_or_setup_agent_success(mock_load_agent_specs): + """Test that verify_agent_exists_or_setup_agent returns agent successfully.""" + # Mock the agent object + mock_agent = MagicMock() + mock_load_agent_specs.return_value = mock_agent # Call the function - result = start_fresh_conversation() + result = verify_agent_exists_or_setup_agent() # Verify the result - assert result == mock_conversation - mock_setup_conversation.assert_called_once_with(None) + assert result == mock_agent + mock_load_agent_specs.assert_called_once_with() @patch('openhands_cli.setup.SettingsScreen') -@patch('openhands_cli.setup.setup_conversation') -def test_start_fresh_conversation_missing_agent_spec( - mock_setup_conversation, +@patch('openhands_cli.setup.load_agent_specs') +def test_verify_agent_exists_or_setup_agent_missing_agent_spec( + mock_load_agent_specs, mock_settings_screen_class ): - """Test that start_fresh_conversation handles MissingAgentSpec exception.""" + """Test that verify_agent_exists_or_setup_agent handles MissingAgentSpec exception.""" # Mock the SettingsScreen instance mock_settings_screen = MagicMock() mock_settings_screen_class.return_value = mock_settings_screen - # Mock setup_conversation to raise MissingAgentSpec on first call, then succeed - mock_conversation = MagicMock() - mock_conversation.id = UUID('12345678-1234-5678-9abc-123456789abc') - mock_setup_conversation.side_effect = [ + # Mock load_agent_specs to raise MissingAgentSpec on first call, then succeed + mock_agent = MagicMock() + mock_load_agent_specs.side_effect = [ MissingAgentSpec("Agent spec missing"), - mock_conversation + mock_agent ] # Call the function - result = start_fresh_conversation() + result = verify_agent_exists_or_setup_agent() # Verify the result - assert result == mock_conversation + assert result == mock_agent # Should be called twice: first fails, second succeeds - assert mock_setup_conversation.call_count == 2 + assert mock_load_agent_specs.call_count == 2 # Settings screen should be called once with first_time=True (new behavior) mock_settings_screen.configure_settings.assert_called_once_with(first_time=True) @@ -59,11 +57,11 @@ def test_start_fresh_conversation_missing_agent_spec( @patch('openhands_cli.agent_chat.exit_session_confirmation') @patch('openhands_cli.agent_chat.get_session_prompter') @patch('openhands_cli.agent_chat.setup_conversation') -@patch('openhands_cli.agent_chat.start_fresh_conversation') +@patch('openhands_cli.agent_chat.verify_agent_exists_or_setup_agent') @patch('openhands_cli.agent_chat.ConversationRunner') def test_new_command_resets_confirmation_mode( mock_runner_cls, - mock_start_fresh_conversation, + mock_verify_agent, mock_setup_conversation, mock_get_session_prompter, mock_exit_confirm, @@ -71,15 +69,17 @@ def test_new_command_resets_confirmation_mode( # Auto-accept the exit prompt to avoid interactive UI and EOFError mock_exit_confirm.return_value = UserConfirmation.ACCEPT - conv1 = MagicMock(); conv1.id = UUID('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa') - conv2 = MagicMock(); conv2.id = UUID('bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb') - mock_start_fresh_conversation.return_value = conv1 - mock_setup_conversation.side_effect = [conv2] + # Mock agent verification to succeed + mock_agent = MagicMock() + mock_verify_agent.return_value = mock_agent - # Distinct runner instances for each conversation + # Mock conversation - only one is created when /new is called + conv1 = MagicMock(); conv1.id = UUID('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa') + mock_setup_conversation.return_value = conv1 + + # One runner instance for the conversation runner1 = MagicMock(); runner1.is_confirmation_mode_active = True - runner2 = MagicMock(); runner2.is_confirmation_mode_active = False - mock_runner_cls.side_effect = [runner1, runner2] + mock_runner_cls.return_value = runner1 # Real session fed by a pipe (no interactive confirmation now) from openhands_cli.user_actions.utils import get_session_prompter as real_get_session_prompter @@ -89,13 +89,12 @@ def test_new_command_resets_confirmation_mode( mock_get_session_prompter.return_value = session from openhands_cli.agent_chat import run_cli_entry - # Trigger /new, then /status, then /exit (exit will be auto-accepted) + # Trigger /new, then /exit (exit will be auto-accepted) for ch in "/new\r/exit\r": pipe.send_text(ch) run_cli_entry(None) - # Assert we switched to a new runner for conv2 - assert mock_runner_cls.call_count == 2 + # Assert we created one runner for the conversation when /new was called + assert mock_runner_cls.call_count == 1 assert mock_runner_cls.call_args_list[0].args[0] is conv1 - assert mock_runner_cls.call_args_list[1].args[0] is conv2 diff --git a/openhands-cli/tests/commands/test_resume_command.py b/openhands-cli/tests/commands/test_resume_command.py new file mode 100644 index 0000000000..e774496d13 --- /dev/null +++ b/openhands-cli/tests/commands/test_resume_command.py @@ -0,0 +1,147 @@ +"""Tests for the /resume command functionality.""" + +from unittest.mock import MagicMock, patch +from uuid import UUID +import pytest +from prompt_toolkit.input.defaults import create_pipe_input +from prompt_toolkit.output.defaults import DummyOutput + +from openhands.sdk.conversation.state import AgentExecutionStatus +from openhands_cli.user_actions import UserConfirmation + + +# ---------- Fixtures & helpers ---------- + +@pytest.fixture +def mock_agent(): + """Mock agent for verification.""" + return MagicMock() + + +@pytest.fixture +def mock_conversation(): + """Mock conversation with default settings.""" + conv = MagicMock() + conv.id = UUID('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa') + return conv + + +@pytest.fixture +def mock_runner(): + """Mock conversation runner.""" + return MagicMock() + + +def run_resume_command_test(commands, agent_status=None, expect_runner_created=True): + """Helper function to run resume command tests with common setup.""" + with patch('openhands_cli.agent_chat.exit_session_confirmation') as mock_exit_confirm, \ + patch('openhands_cli.agent_chat.get_session_prompter') as mock_get_session_prompter, \ + patch('openhands_cli.agent_chat.setup_conversation') as mock_setup_conversation, \ + patch('openhands_cli.agent_chat.verify_agent_exists_or_setup_agent') as mock_verify_agent, \ + patch('openhands_cli.agent_chat.ConversationRunner') as mock_runner_cls: + + # Auto-accept the exit prompt to avoid interactive UI + mock_exit_confirm.return_value = UserConfirmation.ACCEPT + + # Mock agent verification to succeed + mock_agent = MagicMock() + mock_verify_agent.return_value = mock_agent + + # Mock conversation setup + conv = MagicMock() + conv.id = UUID('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa') + if agent_status: + conv.state.agent_status = agent_status + mock_setup_conversation.return_value = conv + + # Mock runner + runner = MagicMock() + runner.conversation = conv + mock_runner_cls.return_value = runner + + # Real session fed by a pipe + from openhands_cli.user_actions.utils import get_session_prompter as real_get_session_prompter + with create_pipe_input() as pipe: + output = DummyOutput() + session = real_get_session_prompter(input=pipe, output=output) + mock_get_session_prompter.return_value = session + + from openhands_cli.agent_chat import run_cli_entry + + # Send commands + for ch in commands: + pipe.send_text(ch) + + # Capture printed output + with patch('openhands_cli.agent_chat.print_formatted_text') as mock_print: + run_cli_entry(None) + + return mock_runner_cls, runner, mock_print + + +# ---------- Warning tests (parametrized) ---------- + +@pytest.mark.parametrize( + "commands,expected_warning,expect_runner_created", + [ + # No active conversation - /resume immediately + ("/resume\r/exit\r", "No active conversation running", False), + # Conversation exists but not in paused state - send message first, then /resume + ("hello\r/resume\r/exit\r", "No paused conversation to resume", True), + ], +) +def test_resume_command_warnings(commands, expected_warning, expect_runner_created): + """Test /resume command shows appropriate warnings.""" + # Set agent status to FINISHED for the "conversation exists but not paused" test + agent_status = AgentExecutionStatus.FINISHED if expect_runner_created else None + + mock_runner_cls, runner, mock_print = run_resume_command_test( + commands, agent_status=agent_status, expect_runner_created=expect_runner_created + ) + + # Verify warning message was printed + warning_calls = [call for call in mock_print.call_args_list + if expected_warning in str(call)] + assert len(warning_calls) > 0, f"Expected warning about {expected_warning}" + + # Verify runner creation expectation + if expect_runner_created: + assert mock_runner_cls.call_count == 1 + runner.process_message.assert_called() + else: + assert mock_runner_cls.call_count == 0 + + +# ---------- Successful resume tests (parametrized) ---------- + +@pytest.mark.parametrize( + "agent_status", + [ + AgentExecutionStatus.PAUSED, + AgentExecutionStatus.WAITING_FOR_CONFIRMATION, + ], +) +def test_resume_command_successful_resume(agent_status): + """Test /resume command successfully resumes paused/waiting conversations.""" + commands = "hello\r/resume\r/exit\r" + + mock_runner_cls, runner, mock_print = run_resume_command_test( + commands, agent_status=agent_status, expect_runner_created=True + ) + + # Verify runner was created and process_message was called + assert mock_runner_cls.call_count == 1 + + # Verify process_message was called twice: once with the initial message, once with None for resume + assert runner.process_message.call_count == 2 + + # Check the calls to process_message + calls = runner.process_message.call_args_list + + # First call should have a message (the "hello" message) + first_call_args = calls[0][0] + assert first_call_args[0] is not None, "First call should have a message" + + # Second call should have None (the /resume command) + second_call_args = calls[1][0] + assert second_call_args[0] is None, "Second call should have None message for resume" \ No newline at end of file diff --git a/openhands-cli/tests/test_confirmation_mode.py b/openhands-cli/tests/test_confirmation_mode.py index 19fc954bf0..fc8fa10c95 100644 --- a/openhands-cli/tests/test_confirmation_mode.py +++ b/openhands-cli/tests/test_confirmation_mode.py @@ -4,6 +4,7 @@ Tests for confirmation mode functionality in OpenHands CLI. """ import os +import uuid from concurrent.futures import ThreadPoolExecutor from typing import Any from unittest.mock import ANY, MagicMock, patch @@ -60,7 +61,7 @@ class TestConfirmationMode: mock_conversation_instance = MagicMock() mock_conversation_class.return_value = mock_conversation_instance - result = setup_conversation() + result = setup_conversation(mock_conversation_id) # Verify conversation was created and returned assert result == mock_conversation_instance @@ -87,7 +88,7 @@ class TestConfirmationMode: # Should raise MissingAgentSpec with pytest.raises(MissingAgentSpec) as exc_info: - setup_conversation() + setup_conversation(uuid.uuid4()) assert 'Agent specification not found' in str(exc_info.value) mock_agent_store_class.assert_called_once()