From 9ec94737ed04658495eefba14149785d77fe75cb Mon Sep 17 00:00:00 2001 From: Ikuo Matsumura Date: Tue, 24 Jun 2025 02:39:38 +0900 Subject: [PATCH] feat(cli): Add vi mode support (#9287) Co-authored-by: Xingyao Wang --- openhands/cli/commands.py | 29 ++++++-- openhands/cli/main.py | 4 +- openhands/cli/settings.py | 11 ++- openhands/cli/tui.py | 37 +++++++--- openhands/core/config/__init__.py | 2 + openhands/core/config/cli_config.py | 9 +++ openhands/core/config/openhands_config.py | 2 + tests/unit/test_cli_commands.py | 22 ++++-- tests/unit/test_cli_tui.py | 22 +++--- tests/unit/test_cli_vi_mode.py | 89 +++++++++++++++++++++++ 10 files changed, 188 insertions(+), 39 deletions(-) create mode 100644 openhands/core/config/cli_config.py create mode 100644 tests/unit/test_cli_vi_mode.py diff --git a/openhands/cli/commands.py b/openhands/cli/commands.py index 8de3d5c750..bcb81fb63e 100644 --- a/openhands/cli/commands.py +++ b/openhands/cli/commands.py @@ -52,6 +52,7 @@ async def handle_commands( if command == '/exit': close_repl = handle_exit_command( + config, event_stream, usage_metrics, sid, @@ -66,7 +67,7 @@ async def handle_commands( handle_status_command(usage_metrics, sid) elif command == '/new': close_repl, new_session_requested = handle_new_command( - event_stream, usage_metrics, sid + config, event_stream, usage_metrics, sid ) elif command == '/settings': await handle_settings_command(config, settings_store) @@ -81,12 +82,16 @@ async def handle_commands( def handle_exit_command( - event_stream: EventStream, usage_metrics: UsageMetrics, sid: str + config: OpenHandsConfig, + event_stream: EventStream, + usage_metrics: UsageMetrics, + sid: str, ) -> bool: close_repl = False confirm_exit = ( - cli_confirm('\nTerminate session?', ['Yes, proceed', 'No, dismiss']) == 0 + cli_confirm(config, '\nTerminate session?', ['Yes, proceed', 'No, dismiss']) + == 0 ) if confirm_exit: @@ -119,7 +124,7 @@ async def handle_init_command( reload_microagents = False if config.runtime == 'local': - init_repo = await init_repository(current_dir) + init_repo = await init_repository(config, current_dir) if init_repo: event_stream.add_event( MessageAction(content=REPO_MD_CREATE_PROMPT), @@ -140,13 +145,17 @@ def handle_status_command(usage_metrics: UsageMetrics, sid: str) -> None: def handle_new_command( - event_stream: EventStream, usage_metrics: UsageMetrics, sid: str + config: OpenHandsConfig, + event_stream: EventStream, + usage_metrics: UsageMetrics, + sid: str, ) -> tuple[bool, bool]: close_repl = False new_session_requested = False new_session_requested = ( cli_confirm( + config, '\nCurrent session will be terminated and you will lose the conversation history.\n\nContinue?', ['Yes, proceed', 'No, dismiss'], ) @@ -171,6 +180,7 @@ async def handle_settings_command( ) -> None: display_settings(config) modify_settings = cli_confirm( + config, '\nWhich settings would you like to modify?', [ 'Basic', @@ -207,7 +217,7 @@ async def handle_resume_command( return close_repl, new_session_requested -async def init_repository(current_dir: str) -> bool: +async def init_repository(config: OpenHandsConfig, current_dir: str) -> bool: repo_file_path = Path(current_dir) / '.openhands' / 'microagents' / 'repo.md' init_repo = False @@ -237,6 +247,7 @@ async def init_repository(current_dir: str) -> bool: init_repo = ( cli_confirm( + config, 'Do you want to re-initialize?', ['Yes, re-initialize', 'No, dismiss'], ) @@ -255,6 +266,7 @@ async def init_repository(current_dir: str) -> bool: init_repo = ( cli_confirm( + config, 'Do you want to proceed?', ['Yes, create', 'No, dismiss'], ) @@ -297,7 +309,10 @@ def check_folder_security_agreement(config: OpenHandsConfig, current_dir: str) - print_formatted_text('') confirm = ( - cli_confirm('Do you wish to continue?', ['Yes, proceed', 'No, exit']) == 0 + cli_confirm( + config, 'Do you wish to continue?', ['Yes, proceed', 'No, exit'] + ) + == 0 ) if confirm: diff --git a/openhands/cli/main.py b/openhands/cli/main.py index cf9c14e301..d8c07ef63d 100644 --- a/openhands/cli/main.py +++ b/openhands/cli/main.py @@ -155,7 +155,7 @@ async def run_session( nonlocal reload_microagents, new_session_requested while True: next_message = await read_prompt_input( - agent_state, multiline=config.cli_multiline_input + config, agent_state, multiline=config.cli_multiline_input ) if not next_message.strip(): @@ -214,7 +214,7 @@ async def run_session( ) return - confirmation_status = await read_confirmation_input() + confirmation_status = await read_confirmation_input(config) if confirmation_status == 'yes' or confirmation_status == 'always': event_stream.add_event( ChangeAgentStateAction(AgentState.USER_CONFIRMED), diff --git a/openhands/cli/settings.py b/openhands/cli/settings.py index e23ba3be91..314797688c 100644 --- a/openhands/cli/settings.py +++ b/openhands/cli/settings.py @@ -135,9 +135,10 @@ async def get_validated_input( return value -def save_settings_confirmation() -> bool: +def save_settings_confirmation(config: OpenHandsConfig) -> bool: return ( cli_confirm( + config, '\nSave new settings? (They will take effect after restart)', ['Yes, save', 'No, discard'], ) @@ -173,6 +174,7 @@ async def modify_llm_settings_basic( # Show verified providers plus "Select another provider" option provider_choices = verified_providers + ['Select another provider'] provider_choice = cli_confirm( + config, '(Step 1/3) Select LLM Provider:', provider_choices, ) @@ -255,6 +257,7 @@ async def modify_llm_settings_basic( ) change_model = ( cli_confirm( + config, 'Do you want to use a different model?', [f'Use {default_model}', 'Select another model'], ) @@ -307,7 +310,7 @@ async def modify_llm_settings_basic( # The try-except block above ensures we either have valid inputs or we've already returned # No need to check for None values here - save_settings = save_settings_confirmation() + save_settings = save_settings_confirmation(config) if not save_settings: return @@ -382,6 +385,7 @@ async def modify_llm_settings_advanced( enable_confirmation_mode = ( cli_confirm( + config, question='(Step 5/6) Confirmation Mode (CTRL-c to cancel):', choices=['Enable', 'Disable'], ) @@ -390,6 +394,7 @@ async def modify_llm_settings_advanced( enable_memory_condensation = ( cli_confirm( + config, question='(Step 6/6) Memory Condensation (CTRL-c to cancel):', choices=['Enable', 'Disable'], ) @@ -406,7 +411,7 @@ async def modify_llm_settings_advanced( # The try-except block above ensures we either have valid inputs or we've already returned # No need to check for None values here - save_settings = save_settings_confirmation() + save_settings = save_settings_confirmation(config) if not save_settings: return diff --git a/openhands/cli/tui.py b/openhands/cli/tui.py index a1c30bbcd5..4a8662a17a 100644 --- a/openhands/cli/tui.py +++ b/openhands/cli/tui.py @@ -520,13 +520,16 @@ class CommandCompleter(Completer): ) -def create_prompt_session() -> PromptSession[str]: - return PromptSession(style=DEFAULT_STYLE) +def create_prompt_session(config: OpenHandsConfig) -> PromptSession[str]: + """Creates a prompt session with VI mode enabled if specified in the config.""" + return PromptSession(style=DEFAULT_STYLE, vi_mode=config.cli.vi_mode) -async def read_prompt_input(agent_state: str, multiline: bool = False) -> str: +async def read_prompt_input( + config: OpenHandsConfig, agent_state: str, multiline: bool = False +) -> str: try: - prompt_session = create_prompt_session() + prompt_session = create_prompt_session(config) prompt_session.completer = ( CommandCompleter(agent_state) if not multiline else None ) @@ -558,9 +561,9 @@ async def read_prompt_input(agent_state: str, multiline: bool = False) -> str: return '/exit' -async def read_confirmation_input() -> str: +async def read_confirmation_input(config: OpenHandsConfig) -> str: try: - prompt_session = create_prompt_session() + prompt_session = create_prompt_session(config) with patch_stdout(): print_formatted_text('') @@ -606,7 +609,9 @@ async def process_agent_pause(done: asyncio.Event, event_stream: EventStream) -> def cli_confirm( - question: str = 'Are you sure?', choices: list[str] | None = None + config: OpenHandsConfig, + question: str = 'Are you sure?', + choices: list[str] | None = None, ) -> int: """Display a confirmation prompt with the given question and choices. @@ -630,15 +635,27 @@ def cli_confirm( kb = KeyBindings() @kb.add('up') - def _(event: KeyPressEvent) -> None: + def _handle_up(event: KeyPressEvent) -> None: selected[0] = (selected[0] - 1) % len(choices) + if config.cli.vi_mode: + + @kb.add('k') + def _handle_k(event: KeyPressEvent) -> None: + selected[0] = (selected[0] - 1) % len(choices) + @kb.add('down') - def _(event: KeyPressEvent) -> None: + def _handle_down(event: KeyPressEvent) -> None: selected[0] = (selected[0] + 1) % len(choices) + if config.cli.vi_mode: + + @kb.add('j') + def _handle_j(event: KeyPressEvent) -> None: + selected[0] = (selected[0] + 1) % len(choices) + @kb.add('enter') - def _(event: KeyPressEvent) -> None: + def _handle_enter(event: KeyPressEvent) -> None: event.app.exit(result=selected[0]) style = Style.from_dict({'selected': COLOR_GOLD, 'unselected': ''}) diff --git a/openhands/core/config/__init__.py b/openhands/core/config/__init__.py index 7af35781eb..cf78955711 100644 --- a/openhands/core/config/__init__.py +++ b/openhands/core/config/__init__.py @@ -1,4 +1,5 @@ from openhands.core.config.agent_config import AgentConfig +from openhands.core.config.cli_config import CLIConfig from openhands.core.config.config_utils import ( OH_DEFAULT_AGENT, OH_MAX_ITERATIONS, @@ -26,6 +27,7 @@ __all__ = [ 'OH_DEFAULT_AGENT', 'OH_MAX_ITERATIONS', 'AgentConfig', + 'CLIConfig', 'OpenHandsConfig', 'MCPConfig', 'LLMConfig', diff --git a/openhands/core/config/cli_config.py b/openhands/core/config/cli_config.py new file mode 100644 index 0000000000..89b6829d3d --- /dev/null +++ b/openhands/core/config/cli_config.py @@ -0,0 +1,9 @@ +from pydantic import BaseModel, Field + + +class CLIConfig(BaseModel): + """Configuration for CLI-specific settings.""" + + vi_mode: bool = Field(default=False) + + model_config = {'extra': 'forbid'} diff --git a/openhands/core/config/openhands_config.py b/openhands/core/config/openhands_config.py index 83b8af1464..69b94db2f1 100644 --- a/openhands/core/config/openhands_config.py +++ b/openhands/core/config/openhands_config.py @@ -5,6 +5,7 @@ from pydantic import BaseModel, Field, SecretStr from openhands.core import logger from openhands.core.config.agent_config import AgentConfig +from openhands.core.config.cli_config import CLIConfig from openhands.core.config.config_utils import ( OH_DEFAULT_AGENT, OH_MAX_ITERATIONS, @@ -109,6 +110,7 @@ class OpenHandsConfig(BaseModel): mcp_host: str = Field(default=f'localhost:{os.getenv("port", 3000)}') mcp: MCPConfig = Field(default_factory=MCPConfig) kubernetes: KubernetesConfig = Field(default_factory=KubernetesConfig) + cli: CLIConfig = Field(default_factory=CLIConfig) defaults_dict: ClassVar[dict] = {} diff --git a/tests/unit/test_cli_commands.py b/tests/unit/test_cli_commands.py index 210c43540a..5ab7d499e7 100644 --- a/tests/unit/test_cli_commands.py +++ b/tests/unit/test_cli_commands.py @@ -50,6 +50,7 @@ class TestHandleCommands: ) mock_handle_exit.assert_called_once_with( + mock_dependencies['config'], mock_dependencies['event_stream'], mock_dependencies['usage_metrics'], mock_dependencies['sid'], @@ -116,6 +117,7 @@ class TestHandleCommands: ) mock_handle_new.assert_called_once_with( + mock_dependencies['config'], mock_dependencies['event_stream'], mock_dependencies['usage_metrics'], mock_dependencies['sid'], @@ -166,6 +168,7 @@ class TestHandleExitCommand: @patch('openhands.cli.commands.cli_confirm') @patch('openhands.cli.commands.display_shutdown_message') def test_exit_with_confirmation(self, mock_display_shutdown, mock_cli_confirm): + config = MagicMock(spec=OpenHandsConfig) event_stream = MagicMock(spec=EventStream) usage_metrics = MagicMock(spec=UsageMetrics) sid = 'test-session-id' @@ -174,7 +177,7 @@ class TestHandleExitCommand: mock_cli_confirm.return_value = 0 # First option, which is "Yes, proceed" # Call the function under test - result = handle_exit_command(event_stream, usage_metrics, sid) + result = handle_exit_command(config, event_stream, usage_metrics, sid) # Verify correct behavior mock_cli_confirm.assert_called_once() @@ -191,6 +194,7 @@ class TestHandleExitCommand: @patch('openhands.cli.commands.cli_confirm') @patch('openhands.cli.commands.display_shutdown_message') def test_exit_without_confirmation(self, mock_display_shutdown, mock_cli_confirm): + config = MagicMock(spec=OpenHandsConfig) event_stream = MagicMock(spec=EventStream) usage_metrics = MagicMock(spec=UsageMetrics) sid = 'test-session-id' @@ -199,7 +203,7 @@ class TestHandleExitCommand: mock_cli_confirm.return_value = 1 # Second option, which is "No, dismiss" # Call the function under test - result = handle_exit_command(event_stream, usage_metrics, sid) + result = handle_exit_command(config, event_stream, usage_metrics, sid) # Verify correct behavior mock_cli_confirm.assert_called_once() @@ -230,6 +234,7 @@ class TestHandleNewCommand: @patch('openhands.cli.commands.cli_confirm') @patch('openhands.cli.commands.display_shutdown_message') def test_new_with_confirmation(self, mock_display_shutdown, mock_cli_confirm): + config = MagicMock(spec=OpenHandsConfig) event_stream = MagicMock(spec=EventStream) usage_metrics = MagicMock(spec=UsageMetrics) sid = 'test-session-id' @@ -238,7 +243,9 @@ class TestHandleNewCommand: mock_cli_confirm.return_value = 0 # First option, which is "Yes, proceed" # Call the function under test - close_repl, new_session = handle_new_command(event_stream, usage_metrics, sid) + close_repl, new_session = handle_new_command( + config, event_stream, usage_metrics, sid + ) # Verify correct behavior mock_cli_confirm.assert_called_once() @@ -256,6 +263,7 @@ class TestHandleNewCommand: @patch('openhands.cli.commands.cli_confirm') @patch('openhands.cli.commands.display_shutdown_message') def test_new_without_confirmation(self, mock_display_shutdown, mock_cli_confirm): + config = MagicMock(spec=OpenHandsConfig) event_stream = MagicMock(spec=EventStream) usage_metrics = MagicMock(spec=UsageMetrics) sid = 'test-session-id' @@ -264,7 +272,9 @@ class TestHandleNewCommand: mock_cli_confirm.return_value = 1 # Second option, which is "No, dismiss" # Call the function under test - close_repl, new_session = handle_new_command(event_stream, usage_metrics, sid) + close_repl, new_session = handle_new_command( + config, event_stream, usage_metrics, sid + ) # Verify correct behavior mock_cli_confirm.assert_called_once() @@ -292,7 +302,7 @@ class TestHandleInitCommand: ) # Verify correct behavior - mock_init_repository.assert_called_once_with(current_dir) + mock_init_repository.assert_called_once_with(config, current_dir) event_stream.add_event.assert_called_once() # Check event is the right type args, kwargs = event_stream.add_event.call_args @@ -320,7 +330,7 @@ class TestHandleInitCommand: ) # Verify correct behavior - mock_init_repository.assert_called_once_with(current_dir) + mock_init_repository.assert_called_once_with(config, current_dir) event_stream.add_event.assert_not_called() assert close_repl is False diff --git a/tests/unit/test_cli_tui.py b/tests/unit/test_cli_tui.py index f5581b785e..1d6464ecaa 100644 --- a/tests/unit/test_cli_tui.py +++ b/tests/unit/test_cli_tui.py @@ -281,7 +281,7 @@ class TestReadConfirmationInput: mock_session.prompt_async.return_value = 'y' mock_create_session.return_value = mock_session - result = await read_confirmation_input() + result = await read_confirmation_input(config=MagicMock(spec=OpenHandsConfig)) assert result == 'yes' @pytest.mark.asyncio @@ -291,7 +291,7 @@ class TestReadConfirmationInput: mock_session.prompt_async.return_value = 'yes' mock_create_session.return_value = mock_session - result = await read_confirmation_input() + result = await read_confirmation_input(config=MagicMock(spec=OpenHandsConfig)) assert result == 'yes' @pytest.mark.asyncio @@ -301,7 +301,7 @@ class TestReadConfirmationInput: mock_session.prompt_async.return_value = 'n' mock_create_session.return_value = mock_session - result = await read_confirmation_input() + result = await read_confirmation_input(config=MagicMock(spec=OpenHandsConfig)) assert result == 'no' @pytest.mark.asyncio @@ -311,7 +311,7 @@ class TestReadConfirmationInput: mock_session.prompt_async.return_value = 'no' mock_create_session.return_value = mock_session - result = await read_confirmation_input() + result = await read_confirmation_input(config=MagicMock(spec=OpenHandsConfig)) assert result == 'no' @pytest.mark.asyncio @@ -321,7 +321,7 @@ class TestReadConfirmationInput: mock_session.prompt_async.return_value = 'a' mock_create_session.return_value = mock_session - result = await read_confirmation_input() + result = await read_confirmation_input(config=MagicMock(spec=OpenHandsConfig)) assert result == 'always' @pytest.mark.asyncio @@ -331,7 +331,7 @@ class TestReadConfirmationInput: mock_session.prompt_async.return_value = 'always' mock_create_session.return_value = mock_session - result = await read_confirmation_input() + result = await read_confirmation_input(config=MagicMock(spec=OpenHandsConfig)) assert result == 'always' @pytest.mark.asyncio @@ -341,7 +341,7 @@ class TestReadConfirmationInput: mock_session.prompt_async.return_value = 'invalid' mock_create_session.return_value = mock_session - result = await read_confirmation_input() + result = await read_confirmation_input(config=MagicMock(spec=OpenHandsConfig)) assert result == 'no' @pytest.mark.asyncio @@ -351,7 +351,7 @@ class TestReadConfirmationInput: mock_session.prompt_async.return_value = '' mock_create_session.return_value = mock_session - result = await read_confirmation_input() + result = await read_confirmation_input(config=MagicMock(spec=OpenHandsConfig)) assert result == 'no' @pytest.mark.asyncio @@ -361,7 +361,7 @@ class TestReadConfirmationInput: mock_session.prompt_async.return_value = None mock_create_session.return_value = mock_session - result = await read_confirmation_input() + result = await read_confirmation_input(config=MagicMock(spec=OpenHandsConfig)) assert result == 'no' @pytest.mark.asyncio @@ -373,7 +373,7 @@ class TestReadConfirmationInput: mock_session.prompt_async.side_effect = KeyboardInterrupt mock_create_session.return_value = mock_session - result = await read_confirmation_input() + result = await read_confirmation_input(config=MagicMock(spec=OpenHandsConfig)) assert result == 'no' @pytest.mark.asyncio @@ -383,5 +383,5 @@ class TestReadConfirmationInput: mock_session.prompt_async.side_effect = EOFError mock_create_session.return_value = mock_session - result = await read_confirmation_input() + result = await read_confirmation_input(config=MagicMock(spec=OpenHandsConfig)) assert result == 'no' diff --git a/tests/unit/test_cli_vi_mode.py b/tests/unit/test_cli_vi_mode.py new file mode 100644 index 0000000000..fbf2b7c150 --- /dev/null +++ b/tests/unit/test_cli_vi_mode.py @@ -0,0 +1,89 @@ +import os +from unittest.mock import ANY, MagicMock, patch + +from openhands.core.config import CLIConfig, OpenHandsConfig + + +class TestCliViMode: + """Test the VI mode feature.""" + + @patch('openhands.cli.tui.PromptSession') + def test_create_prompt_session_vi_mode_enabled(self, mock_prompt_session): + """Test that vi_mode can be enabled.""" + from openhands.cli.tui import create_prompt_session + + config = OpenHandsConfig(cli=CLIConfig(vi_mode=True)) + create_prompt_session(config) + mock_prompt_session.assert_called_with( + style=ANY, + vi_mode=True, + ) + + @patch('openhands.cli.tui.PromptSession') + def test_create_prompt_session_vi_mode_disabled(self, mock_prompt_session): + """Test that vi_mode is disabled by default.""" + from openhands.cli.tui import create_prompt_session + + config = OpenHandsConfig(cli=CLIConfig(vi_mode=False)) + create_prompt_session(config) + mock_prompt_session.assert_called_with( + style=ANY, + vi_mode=False, + ) + + @patch('openhands.cli.tui.Application') + def test_cli_confirm_vi_keybindings_are_added(self, mock_app_class): + """Test that vi keybindings are added to the KeyBindings object.""" + from openhands.cli.tui import cli_confirm + + config = OpenHandsConfig(cli=CLIConfig(vi_mode=True)) + with patch('openhands.cli.tui.KeyBindings', MagicMock()) as mock_key_bindings: + cli_confirm( + config, 'Test question', choices=['Choice 1', 'Choice 2', 'Choice 3'] + ) + # here we are checking if the key bindings are being created + assert mock_key_bindings.call_count == 1 + + # then we check that the key bindings are being added + mock_kb_instance = mock_key_bindings.return_value + assert mock_kb_instance.add.call_count > 0 + + @patch('openhands.cli.tui.Application') + def test_cli_confirm_vi_keybindings_are_not_added(self, mock_app_class): + """Test that vi keybindings are not added when vi_mode is False.""" + from openhands.cli.tui import cli_confirm + + config = OpenHandsConfig(cli=CLIConfig(vi_mode=False)) + with patch('openhands.cli.tui.KeyBindings', MagicMock()) as mock_key_bindings: + cli_confirm( + config, 'Test question', choices=['Choice 1', 'Choice 2', 'Choice 3'] + ) + # here we are checking if the key bindings are being created + assert mock_key_bindings.call_count == 1 + + # then we check that the key bindings are being added + mock_kb_instance = mock_key_bindings.return_value + + # and here we check that the vi key bindings are not being added + for call in mock_kb_instance.add.call_args_list: + assert call[0][0] not in ('j', 'k') + + @patch.dict(os.environ, {}, clear=True) + def test_vi_mode_disabled_by_default(self): + """Test that vi_mode is disabled by default when no env var is set.""" + from openhands.core.config.utils import load_from_env + + config = OpenHandsConfig() + load_from_env(config, os.environ) + assert config.cli.vi_mode is False, 'vi_mode should be False by default' + + @patch.dict(os.environ, {'CLI_VI_MODE': 'True'}) + def test_vi_mode_enabled_from_env(self): + """Test that vi_mode can be enabled from an environment variable.""" + from openhands.core.config.utils import load_from_env + + config = OpenHandsConfig() + load_from_env(config, os.environ) + assert config.cli.vi_mode is True, ( + 'vi_mode should be True when CLI_VI_MODE is set' + )